fix(core): call ngOnDestroy on all services that have it #23755
Conversation
|
You can preview 087701c at https://pr23755-087701c.ngbuilds.io/. |
packages/core/src/view/ng_module.ts
Outdated
| @@ -150,6 +150,11 @@ function _createProviderInstance(ngModule: NgModuleData, providerDef: NgModulePr | |||
| injectable = providerDef.value; | |||
| break; | |||
| } | |||
|
|
|||
| if (injectable !== UNDEFINED_VALUE && injectable !== null && typeof injectable === 'object' && | |||
| !(providerDef.flags & NodeFlags.OnDestroy) && typeof injectable.ngOnDestroy === 'function') { | |||
There was a problem hiding this comment.
Those tests do not look needed: typeof injectable === 'object' && !(providerDef.flags & NodeFlags.OnDestroy)
There was a problem hiding this comment.
The goal of these two checks is to avoid a megamorphic read of ngOnDestroy if there's no chance it will be there (not an object) or if we've already statically detected it there (OnDestroy flag set).
There was a problem hiding this comment.
I will update this so undefined is handled properly.
| 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; |
There was a problem hiding this comment.
not sure why:
- you re-test the presence of
ngOnDestroyhere ? - you have this
Set()
Looks like there is some overlap with changes in ng_module.ts
There was a problem hiding this comment.
I don't understand what you mean by overlap - this is the same file.
We re-test because it's not a guarantee that every call to a factory function will return the same shape object. The flags are shared across all instances of the module, so if the factory ever returns an instance with ngOnDestroy it'll get set, even if other instances it returned previously don't have one. So we need to check the individual instances before we call it.
The Set's job is to guarantee that we don't call ngOnDestroy twice on the same instance in the same injector. Previously this wasn't possible as only useClass providers would call onDestroy, and useClass providers always get their own instance. Now, useFactory and useValue can also give back instances, and could possibly give the same instance for multiple tokens. Therefore it's necessary to track instances and only call ngOnDestroy if it hasn't been called before on the same instance.
Theoretically it's possible that the user could provide the same instance in different injectors, and it would be disposed once per injector, but I think that's reasonable behavior for an edge case.
|
You can preview a8af547 at https://pr23755-a8af547.ngbuilds.io/. |
|
Could we minize runtime perf impact by checking for the method only in the case where we can not determine it statically - ie not when provided as a class ? |
We do - the check for |
|
You can preview a9b31ea at https://pr23755-a9b31ea.ngbuilds.io/. |
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 angular#22466 Fixes angular#22240 Fixes angular#14818
|
You can preview 97b3aa6 at https://pr23755-97b3aa6.ngbuilds.io/. |
alxhub
added
target: major
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
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
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