8000 fix(core): call ngOnDestroy on all services that have it (#23755) · angular/angular@fc03427 · GitHub
[go: up one dir, main page]

Skip to content

Commit fc03427

Browse files
alxhubIgorMinar
authored andcommitted
fix(core): call ngOnDestroy on all services that have it (#23755)
Previously, ngOnDestroy was only called on services which were statically determined to have ngOnDestroy methods. In some cases, such as with services instantiated via factory functions, it's not statically known that the service has an ngOnDestroy method. This commit changes the runtime to look for ngOnDestroy when instantiating all DI tokens, and to call the method if it's present. Fixes #22466 Fixes #22240 Fixes #14818 PR Close #23755
1 parent 77ff72f commit fc03427

File tree

2 files changed

+85
-1
lines changed

2 files changed

+85
-1
lines changed

packages/core/src/view/ng_module.ts

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
8000
@@ -150,6 +150,15 @@ function _createProviderInstance(ngModule: NgModuleData, providerDef: NgModulePr
150150
injectable = providerDef.value;
151151
break;
152152
}
153+
154+
// The read of `ngOnDestroy` here is slightly expensive as it's megamorphic, so it should be
155+
// avoided if possible. The sequence of checks here determines whether ngOnDestroy needs to be
156+
// checked. It might not if the `injectable` isn't an object or if NodeFlags.OnDestroy is already
157+
// set (ngOnDestroy was detected statically).
158+
if (injectable !== UNDEFINED_VALUE && injectable != null && typeof injectable === 'object' &&
159+
!(providerDef.flags & NodeFlags.OnDestroy) && typeof injectable.ngOnDestroy === 'function') {
160+
providerDef.flags |= NodeFlags.OnDestroy;
161+
}
153162
return injectable === undefined ? UNDEFINED_VALUE : injectable;
154163
}
155164

@@ -199,12 +208,17 @@ function _callFactory(ngModule: NgModuleData, factory: any, deps: DepDef[]): any
199208

200209
export function callNgModuleLifecycle(ngModule: NgModuleData, lifecycles: NodeFlags) {
201210
const def = ngModule._def;
211+
const destroyed = new Set<any>();
202212
for (let i = 0; i < def.providers.length; i++) {
203213
const provDef = def.providers[i];
204214
if (provDef.flags & NodeFlags.OnDestroy) {
205215
const instance = ngModule._providers[i];
206216
if (instance && instance !== UNDEFINED_VALUE) {
207-
instance.ngOnDestroy();
217+
const onDestroy: Function|undefined = instance.ngOnDestroy;
218+
if (typeof onDestroy === 'function' && !destroyed.has(instance)) {
219+
onDestroy.apply(instance);
220+
destroyed.add(instance);
221+
}
208222
}
209223
}
210224
}

packages/core/test/view/ng_module_spec.ts

Lines changed: 70 additions & 0 deletions
8000
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,22 @@ function makeProviders(classes: any[], modules: any[]): NgModuleDefinition {
101101
flags: NodeFlags.TypeClassProvider | NodeFlags.LazyProvider, token,
102102
value: token,
103103
}));
104+
return makeModule(modules, providers);
105+
}
106+
107+
function makeFactoryProviders(
108+
factories: {token: any, factory: Function}[], modules: any[]): NgModuleDefinition {
109+
const providers = factories.map((factory, index) => ({
110+
index,
111+
deps: [],
112+
flags: NodeFlags.TypeFactoryProvider | NodeFlags.LazyProvider,
113+
token: factory.token,
114+
value: factory.factory,
115+
}));
116+
return makeModule(modules, providers);
117+
}
118+
119+
function makeModule(modules: any[], providers: NgModuleProviderDef[]): NgModuleDefinition {
104120
const providersByKey: {[key: string]: NgModuleProviderDef} = {};
105121
providers.forEach(provider => providersByKey[tokenKey(provider.token)] = provider);
106122
return {factory: null, providers, providersByKey, modules, isRoot: true};
@@ -155,4 +171,58 @@ describe('NgModuleRef_ injector', () => {
155171

156172
it('injects with the current injector always set',
157173
() => { expect(() => ref.injector.get(UsesInject)).not.toThrow(); });
174+
175+
it('calls ngOnDestroy on services created via factory', () => {
176+
class Module {}
177+
178+
class Service {
179+
static destroyed = 0;
180+
ngOnDestroy(): void { Service.destroyed++; }
181+
}
182+
183+
const ref = createNgModuleRef(
184+
Module, Injector.NULL, [], makeFactoryProviders(
185+
[{
186+
token: Service,
187+
factory: () => new Service(),
188+
}],
189+
[Module]));
190+
191+
expect(ref.injector.get(Service)).toBeDefined();
192+
expect(Service.destroyed).toBe(0);
193+
ref.destroy();
194+
expect(Service.destroyed).toBe(1);
195+
});
196+
197+
it('only calls ngOnDestroy once per instance', () => {
198+
class Module {}
199+
200+
class Service {
201+
static destroyed = 0;
202+
ngOnDestroy(): void { Service.destroyed++; }
203+
}
204+
205+
class OtherToken {}
206+
207+
const instance = new Service();
208+
const ref = createNgModuleRef(
209+
Module, Injector.NULL, [], makeFactoryProviders(
210+
[
211+
{
212+
token: Service,
213+
factory: () => instance,
214+
},
215+
{
216+
token: OtherToken,
217+
factory: () => instance,
218+
}
219+
],
220+
[Module]));
221+
222+
expect(ref.injector.get(Service)).toBe(instance);
223+
expect(ref.injector.get(OtherToken)).toBe(instance);
224+
expect(Service.destroyed).toBe(0);
225+
ref.destroy();
226+
expect(Service.destroyed).toBe(1);
227+
});
158228
});

0 commit comments

Comments
 (0)
0