fix(core): call ngOnDestroy on component level services that are provided using a factory function #28737
Conversation
8baad5e to
194b75a
Compare
10e12f5 to
5ed7c8d
Compare
|
Why was this never reviewed? |
|
I have the same question, why is this not reviewed? |
|
I see that you just added the If you want your PR to be merged, it has to pass all the CI checks. If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help. |
| @@ -312,6 +311,53 @@ import {ARG_TYPE_VALUES, checkNodeInlineOrDynamic, createRootView, createAndGetR | |||
| }); | |||
| }); | |||
|
|
|||
| describe('destroy', () => { | |||
There was a problem hiding this comment.
These tests only verify VE behavior, not Ivy. Support for this setup should probably also be tested using an acceptance test (e.g. in packages/core/test/acceptance/di_spec.ts), as those run against both renderers. I believe you'll find the Ivy behavior to be different, given that this fix only concerns VE at the moment and Ivy also doesn't seem to call ngOnDestroy for component-level providers.
There was a problem hiding this comment.
@llorenspujol just wanted to mention that I had a draft/experimental implementation of this change for Ivy, you can find it here. Feel free to use it if needed (it also contains some tests that you can reuse). Thank you.
packages/core/src/view/provider.ts
Outdated
| def.flags |= NodeFlags.OnDestroy; | ||
| view.def.nodeFlags |= NodeFlags.OnDestroy; | ||
| if (def.parent) { | ||
| def.parent.childFlags |= NodeFlags.OnDestroy; |
There was a problem hiding this comment.
Only updating the flags of the direct parent sounds insufficient to me, as I believe that childFlags is transitive starting from the root node.
There was a problem hiding this comment.
Additionally, once NodeFlags.OnDestroy is set then VE will unconditionally call the ngOnDestroy method:
angular/packages/core/src/view/provider.ts
Lines 575 to 577 in d1ea1f4
This introduces a problem, as def is shared across all component instances but different instances could return different service implementations from their useFactory function, so having the NodeFlags.OnDestroy flag set on the shared definition does no longer guarantee that an ngOnDestroy method is actually present on those different service implementations.
There was a problem hiding this comment.
@JoostK Can you write some use case where these issues happens? for example a test case, or just a draft... I am unable to reproduce both possible problems that you describe.
For example I am debugging all the process and I don't see childFlags transitive from the root node, I may miss something in the process... So, a use case where this issue happens would be very useful!
Thanks in advance!
There was a problem hiding this comment.
@llorenspujol Thanks for looking into it. I'll try provide you with a test later today/tomorrow.
There was a problem hiding this comment.
@llorenspujol Here's the scenarios that would fail in VE but work in Ivy: JoostK@2d6cb9b. I patched the VE implementation to make it work.
(The last of the three tests wouldn't have failed in VE, I added it for good measure)
There was a problem hiding this comment.
Thanks for the reproductions, you really nailed both:), I was miss understanding the 'childs' meaning a little bit:), now it's clear.
|
This PR was discussed in a team meeting and here are the conclusions:
So for this PR it has to be refactored by moving the test to acceptance |
7d2f165 to
6bcd2ce
Compare
Summary(I have discussed this with @AndrewKushnir and leaving our thoughts here for prosperity)
We believe that this PR fixes a bug. The fact that there is code out there which relies on this bug is unfortunate, but this should be treated as a bug fix. We expect that this will have small impact because to see difference in behavior one has to 1) use For existing code the simplest way to refactor for the new correct behavior is to change the injectable to have a ref-counter. Every time an instance of injectable is handed out by |
de97725 to
a1c3f14
Compare
|
Thanks for the summary, it makes sense. So for the refactoring to be enabled, I have pushed new changes adding a I also have modified the forms test that was breaking to use this refactoring, causing it to work as expected. Let me know what do you think about this changes, Thanks! |
Right, so from |
a1c3f14 to
29b7c90
Compare
… and are provided at component level (angular#28738) Fix ngOnDestroy to be called on useFactory providers that implement it and are provided at component level. Fix applies to both ViewEngine and
29b7c90 to
fef12c3
Compare
|
@llorenspujol FYI there's currently an issue with CI, so you won't get the tests green at the moment. |
| /** | ||
| * @description | ||
| * Returns the current number of references to this directive. | ||
| */ | ||
| get refCount() { | ||
| return this._refCount; | ||
| } | ||
|
|
||
| /** | ||
| * @description | ||
| * Adds a reference into the total reference count. | ||
| */ | ||
| addRef() { | ||
| this._refCount++; | ||
| } | ||
|
|
There was a problem hiding this comment.
Is there are reason for these being public? It seems like an implementation detail to me.
There was a problem hiding this comment.
refCount() is an implementation detail but I set it public for consuming it in tests, we could maybe put an underscore to note that it is in fact private.
addRef() Needs to be public AFAIK since we need to be able to execute it when we provide the same instance multiple times in order to correctly do the cleanup.
There was a problem hiding this comment.
My preferred solution would be just having a variable _refCount, for example to avoid having those methods on the 'public api', since they are in fact a hacky way to solve one specific problem.
But I was totally unable to run the build using only that variable, don't know why NgModelGroup._refCount was failing... (the error was _refCount is not a property of NgModelGroup). So I 'imagined' there would be some restriction or some step I don't know and I decided to put those methods to make it work.
There was a problem hiding this comment.
I believe we should revert these changes in Forms based on #28737 (comment). Current understanding is that the problem with re-providing the same instance via useFactory should be taken care of on the apps (not framework) side. However I'd propose to keep a test for now (and mark it with xit vs it) to have more context and we can update or remove the test later.
The next step for this PR is to perform further investigation in Google's codebase (Angular team will handle that) to see if it's feasible to update the code to fix the issue with ngOnDestroy. We'll have more details after investigation and will make a final decision based on this data.
There was a problem hiding this comment.
I may miss something but as far as I know without modifying NgModelGroup or AbstractFormGroupDirective with the capability to count it's references, and execute ngOnDestroy only when there is no reference left is the only way to refactor this use case. It still would have to be handled by the apps that consume it, executing ngModelGroup.addRef() whenever the same instance is provided.
Custom providers from apps could handle this problem the way the want, but in the case of NgModelGroup, apps have no way to prevent calling ngOnDestroy on it, as far as I know.
Other option that comes to mind, without touching framework code, would be making some runtime hack on the __proto__ of NgModelGroup, preventing this way the call of ngOnDestroy, but seems slow and too much complicated.
If there is another way to solve this bug (that I don't see) using refCounting and without touching the framework code, I would like to know, since now only this one's comes to my mind.
Thanks!
There was a problem hiding this comment.
@llorenspujol we'd like to see whether we can refactor all affected usages in Google's codebase to avoid this pattern altogether (so we don't need to instrument the code with new API or monkey-patch field onto instances). We can reconsider the approach after initial investigation.
|
@llorenspujol sorry for the delay. We've discussed this question with the team once again and I'm going to close this PR for now, since landing this change would result in a lot of breakages. We'll keep the original ticket open and look into other possible solutions if we decide to perform some bigger updates to the DI system/API (in which case we can incorporate this behavior). Thank you. |
|
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. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
ngOnDestroy is not called for Services provided via factory function into component level,
Related Issues: 28738; 22466 (this issue is closed, but it only does it for providers provided at module level, not at component level)
Fixes #28738
What is the new behavior?
ngOnDestroy is called on services that are provided with a factory function at component level
Does this PR introduce a breaking change?
Other information
This solution is based on the ones that solved the same issue for the factory function services provided at Module level, see: 5581e97