8000 fix(core): call ngOnDestroy on component level services that are prov… · angular/angular@6bcd2ce · GitHub
[go: up one dir, main page]
8000 Skip to content

Commit 6bcd2ce

Browse files
llorenspujolllorenspujol
committed
fix(core): call ngOnDestroy on component level services that are provided with a factory function
1 parent 6768fe9 commit 6bcd2ce

File tree

5 files changed

+170
-23
lines changed

5 files changed

+170
-23
lines changed

‎packages/core/src/di/r3_injector.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -541,7 +541,7 @@ function isExistingProvider(value: SingleProvider): value is ExistingProvider {
541541
return !!(value && (value as ExistingProvider).useExisting);
542542
}
543543

544-
function isFactoryProvider(value: SingleProvider): value is FactoryProvider {
544+
export function isFactoryProvider(value: SingleProvider): value is FactoryProvider {
545545
return !!(value && (value as FactoryProvider).useFactory);
546546
}
547547

‎packages/core/src/render3/di_setup.ts

Lines changed: 39 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@
88

99

1010
import {resolveForwardRef} from '../di/forward_ref';
11-
import {ClassProvider, Provider} from '../di/interface/provider';
12-
import {isClassProvider, isTypeProvider, providerToFactory} from '../di/r3_injector';
11+
import {ClassProvider, Provider, TypeProvider} from '../di/interface/provider';
12+
import {isClassProvider, isFactoryProvider, isTypeProvider, providerToFactory} from '../di/r3_injector';
1313
import {assertDefined} from '../util/assert';
1414

1515
import {diPublicInInjector, getNodeInjectable, getOrCreateNodeInjectorForNode} from './di';
@@ -179,6 +179,17 @@ function resolveProvider(
179179
}
180180
}
181181

182+
/**
183+
* Function is used as a destroy hook for Factory Providers. This function is invoked in a context
184+
* of a provider instance, checks whether there is an `ngOnDestroy` function and invokes it if it's
185+
* present.
186+
*/
187+
function ngOnDestroyForFactoryProviders(this: any) {
188+
if (this && this.ngOnDestroy) {
189+
this.ngOnDestroy();
190+
}
191+
}
192+
182193
/**
183194
* Registers the `ngOnDestroy` hook of a provider, if the provider supports destroy hooks.
184195
* @param tView `TView` in which to register the hook.
@@ -190,27 +201,36 @@ function resolveProvider(
190201
function registerDestroyHooksIfSupported(
191202
tView: TView, provider: Exclude<Provider, any[]>, contextIndex: number,
192203
indexInFactory?: number) {
193-
const providerIsTypeProvider = isTypeProvider(provider);
194-
if (providerIsTypeProvider || isClassProvider(provider)) {
195-
const prototype = ((provider as ClassProvider).useClass || provider).prototype;
196-
const ngOnDestroy = prototype.ngOnDestroy;
197-
if (ngOnDestroy) {
198-
const hooks = tView.destroyHooks || (tView.destroyHooks = []);
204+
let providerIsClassProvider = false;
205+
let ngOnDestroy;
206+
if (isTypeProvider(provider)) {
207+
ngOnDestroy = (provider as TypeProvider).prototype.ngOnDestroy;
208+
} else if (isClassProvider(provider)) {
209+
providerIsClassProvider = true;
210+
ngOnDestroy = (provider as ClassProvider).useClass.prototype.ngOnDestroy;
211+
} else if (isFactoryProvider(provider)) {
212+
// For factory providers, since we do not have a way to identify whether there is an
213+
// `ngOnDestroy` hook before calling a factory function, we register a special function
214+
// that would be invoked at destroy time and check whether `ngOnDestroy` is present on a
215+
// provider instance (and call the function if it exists).
216+
ngOnDestroy = ngOnDestroyForFactoryProviders;
217+
}
218+
if (ngOnDestroy) {
219+
const hooks = tView.destroyHooks || (tView.destroyHooks = []);
199220

200-
if (!providerIsTypeProvider && ((provider as ClassProvider)).multi) {
201-
ngDevMode &&
202-
assertDefined(
203-
indexInFactory, 'indexInFactory when registering multi factory destroy hook');
204-
const existingCallbacksIndex = hooks.indexOf(contextIndex);
221+
if (providerIsClassProvider && ((provider as ClassProvider)).multi) {
222+
ngDevMode &&
223+
assertDefined(
224+
indexInFactory, 'indexInFactory when registering multi factory destroy hook');
225+
const existingCallbacksIndex = hooks.indexOf(contextIndex);
205226

206-
if (existingCallbacksIndex === -1) {
207-
hooks.push(contextIndex, [indexInFactory, ngOnDestroy]);
208-
} else {
209-
(hooks[existingCallbacksIndex + 1] as DestroyHookData).push(indexInFactory!, ngOnDestroy);
210-
}
227+
if (existingCallbacksIndex === -1) {
228+
hooks.push(contextIndex, [indexInFactory, ngOnDestroy]);
211229
} else {
212-
hooks.push(contextIndex, ngOnDestroy);
230+
(hooks[existingCallbacksIndex + 1] as DestroyHookData).push(indexInFactory!, ngOnDestroy);
213231
}
232+
} else {
233+
hooks.push(contextIndex, ngOnDestroy);
214234
}
215235
}
216236
}

‎packages/core/src/view/provider.ts

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -256,8 +256,20 @@ function _createProviderInstance(view: ViewData, def: NodeDef): any {
256256
return createClass(
257257
view, def.parent!, allowPrivateServices, providerDef!.value, providerDef!.deps);
258258
case NodeFlags.TypeFactoryProvider:
259-
return callFactory(
260-
view, def.parent!, allowPrivateServices, providerDef!.value, providerDef!.deps);
259+
let injectable: any;
260+
injectable = callFactory(
261+
view, def.parent !, allowPrivateServices, providerDef !.value, providerDef !.deps);
262+
// Update def flags if factory injectable has ngOnDestroy function.
263+
if (injectable != null && typeof injectable === 'object' &&
264+
!(def.flags & NodeFlags.OnDestroy) && typeof injectable.ngOnDestroy === 'function') {
265+
def.flags |= NodeFlags.OnDestroy;
266+
view.def.nodeFlags |= NodeFlags.OnDestroy;
267+
if (def.parent) {
268+
def.parent.childFlags |= NodeFlags.OnDestroy;
269+
def.parent.directChildFlags |= NodeFlags.OnDestroy;
270+
}
271+
}
272+
return injectable;
261273
case NodeFlags.TypeUseExistingProvider:
262274
return resolveDep(view, def.parent!, allowPrivateServices, providerDef!.deps[0]);
263275
case NodeFlags.TypeValueProvider:

‎packages/core/test/acceptance/providers_spec.ts

Lines changed: 70 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
*/
88

99
import {CommonModule} from '@angular/common';
10-
import {Component, Directive, forwardRef, Inject, Injectable, InjectionToken, Injector, NgModule, Optional} from '@angular/core';
10+
import {Component, Directive, ElementRef, forwardRef, Inject, Injectable, InjectionToken, Injector, NgModule, Optional} from '@angular/core';
1111
import {inject, TestBed, waitForAsync} from '@angular/core/testing';
1212
import {By} from '@angular/platform-browser';
1313
import {expect} from '@angular/platform-browser/testing/src/matchers';
@@ -508,6 +508,75 @@ describe('providers', () => {
508508
expect(destroyCalls).toBe(0);
509509
});
510510

511+
it('should invoke ngOnDestroy for useFactory providers provided in Component', () => {
512+
const logs: string[] = [];
513+
514+
@Injectable()
515+
class InjectableWithDestroyHook {
516+
ngOnDestroy() {
517+
logs.push('OnDestroy');
518+
}
519+
}
520+
521+
@Component({
522+
template: '',
523+
providers: [{
524+
provide: InjectableWithDestroyHook,
525+
useFactory: () => new InjectableWithDestroyHook(),
526+
}],
527+
})
528+
class App {
529+
constructor(public injectableWithDestroyHook: InjectableWithDestroyHook) {}
530+
}
531+
532+
TestBed.configureTestingModule({declarations: [App]});
533+
534+
const fixture = TestBed.createComponent(App);
535+
fixture.detectChanges();
536+
fixture.destroy();
537+
538+
expect(logs).toEqual(['OnDestroy']);
539+
});
540+
541+
it('should invoke ngOnDestroy for useFactory providers provided in Directive', () => {
542+
const logs: string[] = [];
543+
544+
@Injectable()
545+
class InjectableWithDestroyHook {
546+
ngOnDestroy() {
547+
logs.push('ngOnDestroy');
548+
}
549+
}
550+
551+
@Directive({
552+
selector: '[dir]',
553+
providers: [{
554+
provide: InjectableWithDestroyHook,
555+
useFactory: () => new InjectableWithDestroyHook(),
556+
}],
557+
})
558+
class MyDirective {
559+
constructor(public elRef: ElementRef, public injectableWithDestroyHook: InjectableWithDestroyHook) {}
560+
}
561+
562+
@Component({
563+
selector: 'my-comp',
564+
template: '<div dir></div>',
565+
})
566+
class MyComponent {
567+
}
568+
569+
TestBed.configureTestingModule({
570+
declarations: [MyDirective, MyComponent],
571+
});
572+
573+
const fixture = TestBed.createComponent(MyComponent);
574+
fixture.detectChanges();
575+
fixture.destroy();
576+
577+
expect(logs).toEqual(['ngOnDestroy']);
578+
});
579+
511580
it('should call ngOnDestroy if host component is destroyed', () => {
512581
const logs: string[] = [];
513582

‎packages/core/test/test_bed_spec.ts

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -290,6 +290,52 @@ describe('TestBed', () => {
290290
expect(hello.nativeElement).toHaveText('Hello injected World a second time!');
291291
});
292292

293+
it('should call ngOnDestroy for a service that was overridden with useFactory', () => {
294+
const logs: string[] = [];
295+
296+
@Injectable()
297+
class MyService {
298+
public name = 'MyService';
299+
ngOnDestroy() {
300+
logs.push('MyService.ngOnDestroy');
301+
}
302+
}
303+
304+
class FakeService {
305+
name = 'FakeService';
306+
ngOnDestroy() {
307+
logs.push('FakeService.ngOnDestroy');
308+
}
309+
}
310+
311+
@Component({
312+
selector: 'my-comp',
313+
template: `{{ myService.name }}`,
314+
providers: [MyService],
315+
})
316+
class MyComponent {
317+
constructor(public myService: MyService) {}
318+
}
319+
320+
TestBed.configureTestingModule({
321+
declarations: [MyComponent],
322+
});
323+
324+
TestBed.overrideProvider(MyService, {
325+
useFactory: () => new FakeService(),
326+
});
327+
328+
const fixture = TestBed.createComponent(MyComponent);
329+
fixture.detectChanges();
330+
331+
const service = TestBed.inject(MyService);
332+
expect(service.name).toBe('FakeService');
333+
334+
fixture.destroy();
335+
336+
expect(logs).toEqual(['FakeService.ngOnDestroy']);
337+
});
338+
293339
it('should not call ngOnDestroy for a service that was overridden', () => {
294340
SimpleService.ngOnDestroyCalls = 0;
295341

0 commit comments

Comments
 (0)
0