From e7753131880479f8c084c4233040b1af97e1e5fd Mon Sep 17 00:00:00 2001 From: Marc Laval Date: Mon, 7 Jan 2019 17:07:39 +0100 Subject: [PATCH] fix(ivy): ngOnDestroy hooks should be called on providers (#27955) PR Close #27955 --- packages/core/src/render3/di.ts | 4 + packages/core/src/render3/di_setup.ts | 5 +- packages/core/src/render3/instructions.ts | 3 +- .../core/src/render3/interfaces/injector.ts | 4 + .../change_detection_integration_spec.ts | 25 +++--- .../linker/view_injector_integration_spec.ts | 49 +++++----- packages/core/test/render3/providers_spec.ts | 90 +++++++++++++++++++ 7 files changed, 139 insertions(+), 41 deletions(-) diff --git a/packages/core/src/render3/di.ts b/packages/core/src/render3/di.ts index 63ad94ed1c..362ae8d2ea 100644 --- a/packages/core/src/render3/di.ts +++ b/packages/core/src/render3/di.ts @@ -503,6 +503,10 @@ export function getNodeInjectable( setTNodeAndViewData(tNode, lData); try { value = lData[index] = factory.factory(null, tData, lData, tNode); + const tView = lData[TVIEW]; + if (value && factory.isProvider && value.ngOnDestroy) { + (tView.destroyHooks || (tView.destroyHooks = [])).push(index, value.ngOnDestroy); + } } finally { if (factory.injectImpl) setInjectImplementation(previousInjectImplementation); setIncludeViewProviders(previousIncludeViewProviders); diff --git a/packages/core/src/render3/di_setup.ts b/packages/core/src/render3/di_setup.ts index 58ad14c41a..a60f9c8f8b 100644 --- a/packages/core/src/render3/di_setup.ts +++ b/packages/core/src/render3/di_setup.ts @@ -83,7 +83,8 @@ function resolveProvider( if (isTypeProvider(provider) || !provider.multi) { // Single provider case: the factory is created and pushed immediately - const factory = new NodeInjectorFactory(providerFactory, isViewProvider, directiveInject); + const factory = + new NodeInjectorFactory(providerFactory, isViewProvider, true, directiveInject); const existingFactoryIndex = indexOf( token, tInjectables, isViewProvider ? beginIndex : beginIndex + cptViewProvidersCount, endIndex); @@ -245,7 +246,7 @@ function multiFactory( this: NodeInjectorFactory, _: null, tData: TData, lData: LView, tNode: TElementNode) => any, index: number, isViewProvider: boolean, isComponent: boolean, f: () => any): NodeInjectorFactory { - const factory = new NodeInjectorFactory(factoryFn, isViewProvider, directiveInject); + const factory = new NodeInjectorFactory(factoryFn, isViewProvider, true, directiveInject); factory.multi = []; factory.index = index; factory.componentProviders = 0; diff --git a/packages/core/src/render3/instructions.ts b/packages/core/src/render3/instructions.ts index 4a248e0a67..bfba0db10b 100644 --- a/packages/core/src/render3/instructions.ts +++ b/packages/core/src/render3/instructions.ts @@ -1732,7 +1732,8 @@ function baseResolveDirective( tView: TView, viewData: LView, def: DirectiveDef, directiveFactory: (t: Type| null) => any) { tView.data.push(def); - const nodeInjectorFactory = new NodeInjectorFactory(directiveFactory, isComponentDef(def), null); + const nodeInjectorFactory = + new NodeInjectorFactory(directiveFactory, isComponentDef(def), false, null); tView.blueprint.push(nodeInjectorFactory); viewData.push(nodeInjectorFactory); } diff --git a/packages/core/src/render3/interfaces/injector.ts b/packages/core/src/render3/interfaces/injector.ts index 199b90b0de..64723e4243 100644 --- a/packages/core/src/render3/interfaces/injector.ts +++ b/packages/core/src/render3/interfaces/injector.ts @@ -234,6 +234,10 @@ export class NodeInjectorFactory { * Set to `true` if the token is declared in `viewProviders` (or if it is component). */ isViewProvider: boolean, + /** + * Set to `true` if the token is a provider, and not a directive. + */ + public isProvider: boolean, injectImplementation: null|((token: Type|InjectionToken, flags: InjectFlags) => T)) { this.canSeeViewProviders = isViewProvider; this.injectImpl = injectImplementation; diff --git a/packages/core/test/linker/change_detection_integration_spec.ts b/packages/core/test/linker/change_detection_integration_spec.ts index 606eb36061..1001edbe08 100644 --- a/packages/core/test/linker/change_detection_integration_spec.ts +++ b/packages/core/test/linker/change_detection_integration_spec.ts @@ -1167,23 +1167,22 @@ const TEST_COMPILER_PROVIDERS: Provider[] = [ ]); })); - fixmeIvy('FW-848: ngOnDestroy hooks are not called on providers') - .it('should call ngOnDestroy on an injectable class', fakeAsync(() => { - TestBed.overrideDirective( - TestDirective, {set: {providers: [InjectableWithLifecycle]}}); + it('should call ngOnDestroy on an injectable class', fakeAsync(() => { + TestBed.overrideDirective( + TestDirective, {set: {providers: [InjectableWithLifecycle]}}); - const ctx = createCompFixture('
', TestComponent); + const ctx = createCompFixture('
', TestComponent); - ctx.debugElement.children[0].injector.get(InjectableWithLifecycle); - ctx.detectChanges(false); + ctx.debugElement.children[0].injector.get(InjectableWithLifecycle); + ctx.detectChanges(false); - ctx.destroy(); + ctx.destroy(); - // We don't care about the exact order in this test. - expect(directiveLog.filter(['ngOnDestroy']).sort()).toEqual([ - 'dir.ngOnDestroy', 'injectable.ngOnDestroy' - ]); - })); + // We don't care about the exact order in this test. + expect(directiveLog.filter(['ngOnDestroy']).sort()).toEqual([ + 'dir.ngOnDestroy', 'injectable.ngOnDestroy' + ]); + })); }); }); diff --git a/packages/core/test/linker/view_injector_integration_spec.ts b/packages/core/test/linker/view_injector_integration_spec.ts index 9f89b9854f..c4e0b5f075 100644 --- a/packages/core/test/linker/view_injector_integration_spec.ts +++ b/packages/core/test/linker/view_injector_integration_spec.ts @@ -465,38 +465,37 @@ class TestComp { expect(comp.componentInstance.a).toBe('aValue'); }); - fixmeIvy('FW-848: ngOnDestroy hooks are not called on providers') - .it('should support ngOnDestroy for lazy providers', () => { - let created = false; - let destroyed = false; + it('should support ngOnDestroy for lazy providers', () => { + let created = false; + let destroyed = false; - class SomeInjectable { - constructor() { created = true; } - ngOnDestroy() { destroyed = true; } - } + class SomeInjectable { + constructor() { created = true; } + ngOnDestroy() { destroyed = true; } + } - @Component({providers: [SomeInjectable], template: ''}) - class SomeComp { - } + @Component({providers: [SomeInjectable], template: ''}) + class SomeComp { + } - TestBed.configureTestingModule({declarations: [SomeComp]}); + TestBed.configureTestingModule({declarations: [SomeComp]}); - let compRef = TestBed.createComponent(SomeComp).componentRef; - expect(created).toBe(false); - expect(destroyed).toBe(false); + let compRef = TestBed.createComponent(SomeComp).componentRef; + expect(created).toBe(false); + expect(destroyed).toBe(false); - // no error if the provider was not yet created - compRef.destroy(); - expect(created).toBe(false); - expect(destroyed).toBe(false); + // no error if the provider was not yet created + compRef.destroy(); + expect(created).toBe(false); + expect(destroyed).toBe(false); - compRef = TestBed.createComponent(SomeComp).componentRef; - compRef.injector.get(SomeInjectable); - expect(created).toBe(true); - compRef.destroy(); - expect(destroyed).toBe(true); - }); + compRef = TestBed.createComponent(SomeComp).componentRef; + compRef.injector.get(SomeInjectable); + expect(created).toBe(true); + compRef.destroy(); + expect(destroyed).toBe(true); + }); it('should instantiate view providers lazily', () => { let created = false; diff --git a/packages/core/test/render3/providers_spec.ts b/packages/core/test/render3/providers_spec.ts index a585ae1e67..6eb2a85add 100644 --- a/packages/core/test/render3/providers_spec.ts +++ b/packages/core/test/render3/providers_spec.ts @@ -1265,6 +1265,96 @@ describe('providers', () => { expect(injector.get(Some).location).toEqual('From app component'); }); }); + + describe('lifecycles', () => { + it('should execute ngOnDestroy hooks on providers (and only this one)', () => { + const logs: string[] = []; + + @Injectable() + class InjectableWithLifeCycleHooks { + ngOnChanges() { logs.push('Injectable OnChanges'); } + ngOnInit() { logs.push('Injectable OnInit'); } + ngDoCheck() { logs.push('Injectable DoCheck'); } + ngAfterContentInit() { logs.push('Injectable AfterContentInit'); } + ngAfterContentChecked() { logs.push('Injectable AfterContentChecked'); } + ngAfterViewInit() { logs.push('Injectable AfterViewInit'); } + ngAfterViewChecked() { logs.push('Injectable gAfterViewChecked'); } + ngOnDestroy() { logs.push('Injectable OnDestroy'); } + } + + @Component({template: ``, providers: [InjectableWithLifeCycleHooks]}) + class MyComponent { + constructor(foo: InjectableWithLifeCycleHooks) {} + + static ngComponentDef = defineComponent({ + type: MyComponent, + selectors: [['my-comp']], + factory: () => new MyComponent(directiveInject(InjectableWithLifeCycleHooks)), + consts: 1, + vars: 0, + template: (rf: RenderFlags, ctx: MyComponent) => { + if (rf & RenderFlags.Create) { + element(0, 'span'); + } + }, + features: [ProvidersFeature([InjectableWithLifeCycleHooks])] + }); + } + + @Component({ + template: ` +
+ % if (ctx.condition) { + + % } +
+ `, + }) + class App { + public condition = true; + + static ngComponentDef = defineComponent({ + type: App, + selectors: [['app-cmp']], + factory: () => new App(), + consts: 2, + vars: 0, + template: (rf: RenderFlags, ctx: App) => { + if (rf & RenderFlags.Create) { + elementStart(0, 'div'); + { container(1); } + elementEnd(); + } + if (rf & RenderFlags.Update) { + containerRefreshStart(1); + { + if (ctx.condition) { + let rf1 = embeddedViewStart(1, 2, 1); + { + if (rf1 & RenderFlags.Create) { + element(0, 'my-comp'); + } + } + embeddedViewEnd(); + } + } + containerRefreshEnd(); + } + }, + directives: [MyComponent] + }); + } + + const fixture = new ComponentFixture(App); + fixture.update(); + expect(fixture.html).toEqual('
'); + + fixture.component.condition = false; + fixture.update(); + expect(fixture.html).toEqual('
'); + expect(logs).toEqual(['Injectable OnDestroy']); + }); + }); }); interface ComponentTest { providers?: Provider[];