diff --git a/packages/core/src/view/ng_module.ts b/packages/core/src/view/ng_module.ts index 4a7d6a19ad..9e6bd20305 100644 --- a/packages/core/src/view/ng_module.ts +++ b/packages/core/src/view/ng_module.ts @@ -150,6 +150,15 @@ function _createProviderInstance(ngModule: NgModuleData, providerDef: NgModulePr injectable = providerDef.value; break; } + + // The read of `ngOnDestroy` here is slightly expensive as it's megamorphic, so it should be + // avoided if possible. The sequence of checks here determines whether ngOnDestroy needs to be + // checked. It might not if the `injectable` isn't an object or if NodeFlags.OnDestroy is already + // set (ngOnDestroy was detected statically). + if (injectable !== UNDEFINED_VALUE && injectable != null && typeof injectable === 'object' && + !(providerDef.flags & NodeFlags.OnDestroy) && typeof injectable.ngOnDestroy === 'function') { + providerDef.flags |= NodeFlags.OnDestroy; + } return injectable === undefined ? UNDEFINED_VALUE : injectable; } @@ -199,12 +208,17 @@ function _callFactory(ngModule: NgModuleData, factory: any, deps: DepDef[]): any export function callNgModuleLifecycle(ngModule: NgModuleData, lifecycles: NodeFlags) { const def = ngModule._def; + const destroyed = new Set(); for (let i = 0; i < def.providers.length; i++) { const provDef = def.providers[i]; if (provDef.flags & NodeFlags.OnDestroy) { const instance = ngModule._providers[i]; if (instance && instance !== UNDEFINED_VALUE) { - instance.ngOnDestroy(); + const onDestroy: Function|undefined = instance.ngOnDestroy; + if (typeof onDestroy === 'function' && !destroyed.has(instance)) { + onDestroy.apply(instance); + destroyed.add(instance); + } } } } diff --git a/packages/core/test/view/ng_module_spec.ts b/packages/core/test/view/ng_module_spec.ts index 4e89b95ae0..6e3c1fa9a1 100644 --- a/packages/core/test/view/ng_module_spec.ts +++ b/packages/core/test/view/ng_module_spec.ts @@ -101,6 +101,22 @@ function makeProviders(classes: any[], modules: any[]): NgModuleDefinition { flags: NodeFlags.TypeClassProvider | NodeFlags.LazyProvider, token, value: token, })); + return makeModule(modules, providers); +} + +function makeFactoryProviders( + factories: {token: any, factory: Function}[], modules: any[]): NgModuleDefinition { + const providers = factories.map((factory, index) => ({ + index, + deps: [], + flags: NodeFlags.TypeFactoryProvider | NodeFlags.LazyProvider, + token: factory.token, + value: factory.factory, + })); + return makeModule(modules, providers); +} + +function makeModule(modules: any[], providers: NgModuleProviderDef[]): NgModuleDefinition { const providersByKey: {[key: string]: NgModuleProviderDef} = {}; providers.forEach(provider => providersByKey[tokenKey(provider.token)] = provider); return {factory: null, providers, providersByKey, modules, isRoot: true}; @@ -155,4 +171,58 @@ describe('NgModuleRef_ injector', () => { it('injects with the current injector always set', () => { expect(() => ref.injector.get(UsesInject)).not.toThrow(); }); + + it('calls ngOnDestroy on services created via factory', () => { + class Module {} + + class Service { + static destroyed = 0; + ngOnDestroy(): void { Service.destroyed++; } + } + + const ref = createNgModuleRef( + Module, Injector.NULL, [], makeFactoryProviders( + [{ + token: Service, + factory: () => new Service(), + }], + [Module])); + + expect(ref.injector.get(Service)).toBeDefined(); + expect(Service.destroyed).toBe(0); + ref.destroy(); + expect(Service.destroyed).toBe(1); + }); + + it('only calls ngOnDestroy once per instance', () => { + class Module {} + + class Service { + static destroyed = 0; + ngOnDestroy(): void { Service.destroyed++; } + } + + class OtherToken {} + + const instance = new Service(); + const ref = createNgModuleRef( + Module, Injector.NULL, [], makeFactoryProviders( + [ + { + token: Service, + factory: () => instance, + }, + { + token: OtherToken, + factory: () => instance, + } + ], + [Module])); + + expect(ref.injector.get(Service)).toBe(instance); + expect(ref.injector.get(OtherToken)).toBe(instance); + expect(Service.destroyed).toBe(0); + ref.destroy(); + expect(Service.destroyed).toBe(1); + }); });