diff --git a/packages/core/src/render3/di_setup.ts b/packages/core/src/render3/di_setup.ts index 1666dbcae5..483ae76f44 100644 --- a/packages/core/src/render3/di_setup.ts +++ b/packages/core/src/render3/di_setup.ts @@ -81,26 +81,18 @@ function resolveProvider( const cptViewProvidersCount = tNode.providerIndexes >> TNodeProviderIndexes.CptViewProvidersCountShift; - if (isClassProvider(provider) || isTypeProvider(provider)) { - const prototype = ((provider as ClassProvider).useClass || provider).prototype; - const ngOnDestroy = prototype.ngOnDestroy; - - if (ngOnDestroy) { - (tView.destroyHooks || (tView.destroyHooks = [])).push(tInjectables.length, ngOnDestroy); - } - } - if (isTypeProvider(provider) || !provider.multi) { // Single provider case: the factory is created and pushed immediately const factory = new NodeInjectorFactory(providerFactory, isViewProvider, ɵɵdirectiveInject); const existingFactoryIndex = indexOf( token, tInjectables, isViewProvider ? beginIndex : beginIndex + cptViewProvidersCount, endIndex); - if (existingFactoryIndex == -1) { + if (existingFactoryIndex === -1) { diPublicInInjector( getOrCreateNodeInjectorForNode( tNode as TElementNode | TContainerNode | TElementContainerNode, lView), tView, token); + registerDestroyHooksIfSupported(tView, provider, tInjectables.length); tInjectables.push(token); tNode.directiveStart++; tNode.directiveEnd++; @@ -157,6 +149,7 @@ function resolveProvider( if (!isViewProvider && doesViewProvidersFactoryExist) { lInjectablesBlueprint[existingViewProvidersFactoryIndex].providerFactory = factory; } + registerDestroyHooksIfSupported(tView, provider, tInjectables.length); tInjectables.push(token); tNode.directiveStart++; tNode.directiveEnd++; @@ -167,6 +160,10 @@ function resolveProvider( lView.push(factory); } else { // Cases 1.b and 2.b + registerDestroyHooksIfSupported( + tView, provider, existingProvidersFactoryIndex > -1 ? + existingProvidersFactoryIndex : + existingViewProvidersFactoryIndex); multiFactoryAdd( lInjectablesBlueprint ![isViewProvider ? existingViewProvidersFactoryIndex : existingProvidersFactoryIndex], providerFactory, !isViewProvider && isComponent); @@ -178,6 +175,24 @@ function resolveProvider( } } +/** + * Registers the `ngOnDestroy` hook of a provider, if the provider supports destroy hooks. + * @param tView `TView` in which to register the hook. + * @param provider Provider whose hook should be registered. + * @param contextIndex Index under which to find the context for the hook when it's being invoked. + */ +function registerDestroyHooksIfSupported( + tView: TView, provider: Exclude, contextIndex: number) { + const providerIsTypeProvider = isTypeProvider(provider); + if (providerIsTypeProvider || isClassProvider(provider)) { + const prototype = ((provider as ClassProvider).useClass || provider).prototype; + const ngOnDestroy = prototype.ngOnDestroy; + if (ngOnDestroy) { + (tView.destroyHooks || (tView.destroyHooks = [])).push(contextIndex, ngOnDestroy); + } + } +} + /** * Add a factory in a multi factory. */ diff --git a/packages/core/test/acceptance/providers_spec.ts b/packages/core/test/acceptance/providers_spec.ts index 581b2080f4..2c9e295724 100644 --- a/packages/core/test/acceptance/providers_spec.ts +++ b/packages/core/test/acceptance/providers_spec.ts @@ -223,6 +223,129 @@ describe('providers', () => { expect(logs).toEqual(['OnDestroy Existing']); }); + it('should invoke ngOnDestroy with the correct context when providing a type provider multiple times on the same node', + () => { + const resolvedServices: (DestroyService | undefined)[] = []; + const destroyContexts: (DestroyService | undefined)[] = []; + let parentService: DestroyService|undefined; + let childService: DestroyService|undefined; + + @Injectable() + class DestroyService { + constructor() { resolvedServices.push(this); } + ngOnDestroy() { destroyContexts.push(this); } + } + + @Directive({selector: '[dir-one]', providers: [DestroyService]}) + class DirOne { + constructor(service: DestroyService) { childService = service; } + } + + @Directive({selector: '[dir-two]', providers: [DestroyService]}) + class DirTwo { + constructor(service: DestroyService) { childService = service; } + } + + @Component({template: '
', providers: [DestroyService]}) + class App { + constructor(service: DestroyService) { parentService = service; } + } + + TestBed.configureTestingModule({declarations: [App, DirOne, DirTwo]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + fixture.destroy(); + + expect(parentService).toBeDefined(); + expect(childService).toBeDefined(); + expect(parentService).not.toBe(childService); + expect(resolvedServices).toEqual([parentService, childService]); + expect(destroyContexts).toEqual([parentService, childService]); + }); + + onlyInIvy('Destroy hook of useClass provider is invoked correctly') + .it('should invoke ngOnDestroy with the correct context when providing a class provider multiple times on the same node', + () => { + const resolvedServices: (DestroyService | undefined)[] = []; + const destroyContexts: (DestroyService | undefined)[] = []; + const token = new InjectionToken('token'); + let parentService: DestroyService|undefined; + let childService: DestroyService|undefined; + + @Injectable() + class DestroyService { + constructor() { resolvedServices.push(this); } + ngOnDestroy() { destroyContexts.push(this); } + } + + @Directive( + {selector: '[dir-one]', providers: [{provide: token, useClass: DestroyService}]}) + class DirOne { + constructor(@Inject(token) service: DestroyService) { childService = service; } + } + + @Directive( + {selector: '[dir-two]', providers: [{provide: token, useClass: DestroyService}]}) + class DirTwo { + constructor(@Inject(token) service: DestroyService) { childService = service; } + } + + @Component({ + template: '
', + providers: [{provide: token, useClass: DestroyService}] + }) + class App { + constructor(@Inject(token) service: DestroyService) { parentService = service; } + } + + TestBed.configureTestingModule({declarations: [App, DirOne, DirTwo]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + fixture.destroy(); + + expect(parentService).toBeDefined(); + expect(childService).toBeDefined(); + expect(parentService).not.toBe(childService); + expect(resolvedServices).toEqual([parentService, childService]); + expect(destroyContexts).toEqual([parentService, childService]); + }); + + onlyInIvy('ngOnDestroy hooks for multi providers were not supported in ViewEngine') + .it('should not invoke ngOnDestroy on multi providers', () => { + // TODO(FW-1866): currently we only assert that the hook was called, + // but we should also be checking that the correct context was passed in. + let destroyCalls = 0; + const SERVICES = new InjectionToken('SERVICES'); + + @Injectable() + class DestroyService { + ngOnDestroy() { destroyCalls++; } + } + + @Injectable() + class OtherDestroyService { + ngOnDestroy() { destroyCalls++; } + } + + @Component({ + template: '
', + providers: [ + {provide: SERVICES, useClass: DestroyService, multi: true}, + {provide: SERVICES, useClass: OtherDestroyService, multi: true}, + ] + }) + class App { + constructor(@Inject(SERVICES) s: any) {} + } + + TestBed.configureTestingModule({declarations: [App]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + fixture.destroy(); + + expect(destroyCalls).toBe(2); + }); + }); describe('components and directives', () => {