Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(forms): FormControlName directive not cleaning up on destroy #37924

Closed
wants to merge 1 commit into from

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Jul 4, 2020

Fixes a memory leak caused by the FormControlName directive which wasn't cleaning up after itself on destroy. It ended up retaining a reference to the detached DOM node that the directive is applied to.

Fixes #37431.

Fixes a memory leak caused by the `FormControlName` directive which wasn't cleaning up after itself on destroy. It ended up retaining a reference to the detached DOM node that the directive is applied to.

Fixes angular#37431.
@crisbeto crisbeto marked this pull request as ready for review July 4, 2020 11:40
@pullapprove pullapprove bot requested a review from AndrewKushnir July 4, 2020 11:40
@crisbeto crisbeto added area: forms action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Jul 4, 2020
@ngbot ngbot bot modified the milestone: needsTriage Jul 4, 2020
@AndrewKushnir
Copy link
Contributor

Hi @crisbeto, thanks for creating this PR.

I was working on that problem as a part of #37566 (it's in a Draft mode, since I was checking how things work with Google apps). This part of the fix from #37566 is very similar to what you implemented (which confirms the overall direction), but the idea that I had was to keep the setUpControl and cleanUpControl (which performs reverse operation from setUpControl) function calls on the same logical level (i.e. within the same class). Also it turned out that the cleanUpControl function itself is not working correctly in all cases (see additional changes in #37566), for example it doesn't clear validators attached to the FormControl instances - I plan to fix that too after landing #37881 (which should allow more granular operations with the list of validators). The delta between our fixes is that you also clear the control field and I should look into that, so let's keep this PR open and I will share more info after additional investigation.

Thank you.

@crisbeto
Copy link
Member Author

Closing in favor of #37566.

@crisbeto crisbeto closed this Jul 18, 2020
@rppig42
Copy link

rppig42 commented Jul 23, 2020

Hi there, would this patch be merged into Angular@9.x?

And thank you for your work

@crisbeto
Copy link
Member Author

It might be better to ask on #37566 since this PR didn't end up getting merged.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: review The PR is still awaiting reviews from at least one requested reviewer area: forms cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using ngIf, DOM nodes cannot be garbage collected because of form control
4 participants