fix(forms): more precise control cleanup #39623
Conversation
AndrewKushnir
added
type: bug/fix
action: review
AndrewKushnir
force-pushed
the
forms-cleanup-control
branch
2 times, most recently
from
2d1be6a
to
bd54312
Compare
Currently when an instance of the `FormControlName` directive is destroyed, the Forms package invokes the `cleanUpControl` to clear all directive-specific logic (such as validators, onChange handlers, etc) from a bound control. The logic of the `cleanUpControl` function should revert all setup performed by the `setUpControl` function. However the `cleanUpControl` is too aggressive and removes all callbacks related to the onChange and disabled state handling. This is causing problems when a form control is bound to multiple FormControlName` directives, causing other instances of that directive to stop working correctly when the first one is destroyed. This commit updates the cleanup logic to only remove callbacks added while setting up a control for a given directive instance. The fix is needed to allow adding `cleanUpControl` function to other places where cleanup is needed (missing this function calls in some other places causes memory leak issues).
AndrewKushnir
force-pushed
the
forms-cleanup-control
branch
from
bd54312
to
6e1f5b9
Compare
AndrewKushnir
added
action: presubmit
|
Thanks for review @petebacondarwin, great feedback! I've applied the necessary changes, please have a look when you get a sec.
Yeah, there are no substantial differences. I can do some cleanup by moving things around once this change (and the followup PR) lands. (Note to self: I've also started a new presubmit + global presubmit) |
|
Thanks for the review @petebacondarwin 👍 Global presubmit went well (no regressions), so I'm adding this PR to the merge queue. Thank you. |
AndrewKushnir
added
action: merge
Currently when an instance of the `FormControlName` directive is destroyed, the Forms package invokes the `cleanUpControl` to clear all directive-specific logic (such as validators, onChange handlers, etc) from a bound control. The logic of the `cleanUpControl` function should revert all setup performed by the `setUpControl` function. However the `cleanUpControl` is too aggressive and removes all callbacks related to the onChange and disabled state handling. This is causing problems when a form control is bound to multiple FormControlName` directives, causing other instances of that directive to stop working correctly when the first one is destroyed. This commit updates the cleanup logic to only remove callbacks added while setting up a control for a given directive instance. The fix is needed to allow adding `cleanUpControl` function to other places where cleanup is needed (missing this function calls in some other places causes memory leak issues). PR Close #39623
|
@AndrewKushnir is this fixing #20007 by any chance 🙏 ? |
|
@maxime1992 thanks for the question. No, this PR would not help with memory management, but it's a prerequisite for #39235, which should address the memory leak issue caused by the |
|
That. is. awesome. Thank you so much for looking into this 🙏 🎉 |
|
@maxime1992 JFYI I came across another issue while testing #39235 with the |
Currently when an instance of the `FormControlName` directive is destroyed, the Forms package invokes the `cleanUpControl` to clear all directive-specific logic (such as validators, onChange handlers, etc) from a bound control. The logic of the `cleanUpControl` function should revert all setup performed by the `setUpControl` function. However the `cleanUpControl` is too aggressive and removes all callbacks related to the onChange and disabled state handling. This is causing problems when a form control is bound to multiple FormControlName` directives, causing other instances of that directive to stop working correctly when the first one is destroyed. This commit updates the cleanup logic to only remove callbacks added while setting up a control for a given directive instance. The fix is needed to allow adding `cleanUpControl` function to other places where cleanup is needed (missing this function calls in some other places causes memory leak issues). PR Close angular#39623
|
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. |
pullapprove
bot
added
area: build & ci
Currently when an instance of the
FormControlNamedirective is destroyed, the Forms package invokesthe
cleanUpControlto clear all directive-specific logic (such as validators, onChange handlers,etc) from a bound control. The logic of the
cleanUpControlfunction should revert all setupperformed by the
setUpControlfunction. However thecleanUpControlis too aggressive and removesall callbacks related to the onChange and disabled state handling. This is causing problems when
a form control is bound to multiple FormControlName` directives, causing other instances of that
directive to stop working correctly when the first one is destroyed.
This commit updates the cleanup logic to only remove callbacks added while setting up a control
for a given directive instance.
The fix is needed to allow adding
cleanUpControlfunction to other places where cleanup is needed(missing this function calls in some other places causes memory leak issues).
PR Type
What kind of change does this PR introduce?
Does this PR introduce a breaking change?