From ab2bf8339890ce3a4bef5bfbcee4c735cd57aac2 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Wed, 16 Jan 2019 20:35:56 +0100 Subject: [PATCH] fix(ivy): destroy injector when module is destroyed (#27793) Destroys the module's injector when an `NgModule` is destroyed which in turn calls the `ngOnDestroy` methods on the instantiated providers. This PR resolves FW-739. PR Close #27793 --- packages/core/src/di/r3_injector.ts | 7 +- packages/core/src/render3/component_ref.ts | 2 +- packages/core/src/render3/ng_module_ref.ts | 8 +- .../test/linker/ng_module_integration_spec.ts | 78 +++++++++---------- .../src/common/downgrade_component_adapter.ts | 4 +- 5 files changed, 50 insertions(+), 49 deletions(-) diff --git a/packages/core/src/di/r3_injector.ts b/packages/core/src/di/r3_injector.ts index e0253a464e..2f16c70f6d 100644 --- a/packages/core/src/di/r3_injector.ts +++ b/packages/core/src/di/r3_injector.ts @@ -102,7 +102,8 @@ export class R3Injector { /** * Flag indicating that this injector was previously destroyed. */ - private destroyed = false; + get destroyed(): boolean { return this._destroyed; } + private _destroyed = false; constructor( def: InjectorType, additionalProviders: StaticProvider[]|null, @@ -138,7 +139,7 @@ export class R3Injector { this.assertNotDestroyed(); // Set destroyed = true first, in case lifecycle hooks re-enter destroy(). - this.destroyed = true; + this._destroyed = true; try { // Call all the lifecycle hooks. this.onDestroy.forEach(service => service.ngOnDestroy()); @@ -189,7 +190,7 @@ export class R3Injector { } private assertNotDestroyed(): void { - if (this.destroyed) { + if (this._destroyed) { throw new Error('Injector has already been destroyed.'); } } diff --git a/packages/core/src/render3/component_ref.ts b/packages/core/src/render3/component_ref.ts index 8a37abae32..be48fabeed 100644 --- a/packages/core/src/render3/component_ref.ts +++ b/packages/core/src/render3/component_ref.ts @@ -273,7 +273,7 @@ export class ComponentRef extends viewEngine_ComponentRef { ngDevMode && assertDefined(this.destroyCbs, 'NgModule already destroyed'); this.destroyCbs !.forEach(fn => fn()); this.destroyCbs = null; - this.hostView.destroy(); + !this.hostView.destroyed && this.hostView.destroy(); } onDestroy(callback: () => void): void { ngDevMode && assertDefined(this.destroyCbs, 'NgModule already destroyed'); diff --git a/packages/core/src/render3/ng_module_ref.ts b/packages/core/src/render3/ng_module_ref.ts index 17727997c3..6442393b70 100644 --- a/packages/core/src/render3/ng_module_ref.ts +++ b/packages/core/src/render3/ng_module_ref.ts @@ -9,7 +9,7 @@ import {INJECTOR, Injector} from '../di/injector'; import {InjectFlags} from '../di/interface/injector'; import {StaticProvider} from '../di/interface/provider'; -import {createInjector} from '../di/r3_injector'; +import {R3Injector, createInjector} from '../di/r3_injector'; import {Type} from '../interface/type'; import {ComponentFactoryResolver as viewEngine_ComponentFactoryResolver} from '../linker/component_factory_resolver'; import {InternalNgModuleRef, NgModuleFactory as viewEngine_NgModuleFactory, NgModuleRef as viewEngine_NgModuleRef} from '../linker/ng_module_factory'; @@ -32,7 +32,7 @@ export class NgModuleRef extends viewEngine_NgModuleRef implements Interna // tslint:disable-next-line:require-internal-with-underscore _bootstrapComponents: Type[] = []; // tslint:disable-next-line:require-internal-with-underscore - _r3Injector: Injector; + _r3Injector: R3Injector; injector: Injector = this; instance: T; destroyCbs: (() => void)[]|null = []; @@ -52,7 +52,7 @@ export class NgModuleRef extends viewEngine_NgModuleRef implements Interna }, COMPONENT_FACTORY_RESOLVER ]; - this._r3Injector = createInjector(ngModuleType, _parent, additionalProviders); + this._r3Injector = createInjector(ngModuleType, _parent, additionalProviders) as R3Injector; this.instance = this.get(ngModuleType); } @@ -70,6 +70,8 @@ export class NgModuleRef extends viewEngine_NgModuleRef implements Interna destroy(): void { ngDevMode && assertDefined(this.destroyCbs, 'NgModule already destroyed'); + const injector = this._r3Injector; + !injector.destroyed && injector.destroy(); this.destroyCbs !.forEach(fn => fn()); this.destroyCbs = null; } diff --git a/packages/core/test/linker/ng_module_integration_spec.ts b/packages/core/test/linker/ng_module_integration_spec.ts index 36c52354dc..53512bdde1 100644 --- a/packages/core/test/linker/ng_module_integration_spec.ts +++ b/packages/core/test/linker/ng_module_integration_spec.ts @@ -1045,55 +1045,53 @@ function declareTests(config?: {useJit: boolean}) { expect(created).toBe(false); }); - fixmeIvy('FW-739: TestBed: destroy on NgModuleRef is not being called') - .it('should support ngOnDestroy on any provider', () => { - let destroyed = false; + it('should support ngOnDestroy on any provider', () => { + let destroyed = false; - class SomeInjectable { - ngOnDestroy() { destroyed = true; } - } + class SomeInjectable { + ngOnDestroy() { destroyed = true; } + } - @NgModule({providers: [SomeInjectable]}) - class SomeModule { - // Inject SomeInjectable to make it eager... - constructor(i: SomeInjectable) {} - } + @NgModule({providers: [SomeInjectable]}) + class SomeModule { + // Inject SomeInjectable to make it eager... + constructor(i: SomeInjectable) {} + } - const moduleRef = createModule(SomeModule); - expect(destroyed).toBe(false); - moduleRef.destroy(); - expect(destroyed).toBe(true); - }); + const moduleRef = createModule(SomeModule); + expect(destroyed).toBe(false); + moduleRef.destroy(); + expect(destroyed).toBe(true); + }); - fixmeIvy('FW-739: TestBed: destroy on NgModuleRef is not being called') - .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; } + } - @NgModule({providers: [SomeInjectable]}) - class SomeModule { - } + @NgModule({providers: [SomeInjectable]}) + class SomeModule { + } - let moduleRef = createModule(SomeModule); - expect(created).toBe(false); - expect(destroyed).toBe(false); + let moduleRef = createModule(SomeModule); + expect(created).toBe(false); + expect(destroyed).toBe(false); - // no error if the provider was not yet created - moduleRef.destroy(); - expect(created).toBe(false); - expect(destroyed).toBe(false); + // no error if the provider was not yet created + moduleRef.destroy(); + expect(created).toBe(false); + expect(destroyed).toBe(false); - moduleRef = createModule(SomeModule); - moduleRef.injector.get(SomeInjectable); - expect(created).toBe(true); - moduleRef.destroy(); - expect(destroyed).toBe(true); - }); + moduleRef = createModule(SomeModule); + moduleRef.injector.get(SomeInjectable); + expect(created).toBe(true); + moduleRef.destroy(); + expect(destroyed).toBe(true); + }); }); describe('imported and exported modules', () => { diff --git a/packages/upgrade/src/common/downgrade_component_adapter.ts b/packages/upgrade/src/common/downgrade_component_adapter.ts index b11b8bca50..b3c0f56252 100644 --- a/packages/upgrade/src/common/downgrade_component_adapter.ts +++ b/packages/upgrade/src/common/downgrade_component_adapter.ts @@ -213,6 +213,7 @@ export class DowngradeComponentAdapter { } registerCleanup() { + const testabilityRegistry = this.componentRef.injector.get(TestabilityRegistry); const destroyComponentRef = this.wrapCallback(() => this.componentRef.destroy()); let destroyed = false; @@ -220,8 +221,7 @@ export class DowngradeComponentAdapter { this.componentScope.$on('$destroy', () => { if (!destroyed) { destroyed = true; - this.componentRef.injector.get(TestabilityRegistry) - .unregisterApplication(this.componentRef.location.nativeElement); + testabilityRegistry.unregisterApplication(this.componentRef.location.nativeElement); destroyComponentRef(); } });