fix(forms): clear parent when removing controls (#29517)

[mbinic]

PR Checklist

• The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• angular.io application / infrastructure changes
• Other... Please describe:

What is the current behavior?

When a control is removed from a FormGroup (removeControl), or replaced by another control (setControl), that control's parent is not cleared, i.e. the removed control continues to reference the old parent.

Issue Number: #29517

What is the new behavior?

When a control is removed or replaced, its parent is set to null.

Does this PR introduce a breaking change?

• Yes
• No

If you were relying on the removed/replaced controls to retain the old parent reference, this would be a breaking change. This kind of usage seems like a hack, but to get it working, one could store the old parent reference prior to calling removeControl/setControl or in an overridden setParent method.

Other information

Docs for the new unregisterControl method have been added in the code file (forms/model.ts).

[mbinic]

The first failing check "ci/circleci:test" is a result of the public API change (public_api_guard:forms_api), which is expected and should be fine.
The second one, "ci/circleci: test_docs_examples", doesn't seem to be related to my changes as the failing test is "Lifecycle hooks - should support DoCheck hook". It might be that my branch just inherited something faulty from master.

[mbinic]

@fw-forms, please review, I'd be happy to address any issues and concerns :)

[itbrandonsilva]

Is there any particular reason the idea proposed by the PR is not valid (not taking into account CI checks)?

[mbinic]

Hi @itbrandonsilva,
When writing this, I had no doubts apart from undefined vs null. But, we should most probably go with whatever is most consistent with the rest of the codebase.

TLDR:
One might lean towards undefined because:

1. When removing a child, its parent is no longer defined, rather than it being a child of null.
2. When/if someone decides to serialize the state, they would probably omit the parent property, rather than including parent: null.
   Otherwise, I see no practical implications, as one could just use the same check (!parent) and be covered for both.

[AndrewKushnir]

Hi @mbinic, thanks for creating this PR.

I looked at the PR description and the issue that you refer to and I'd like to discuss it more. Currently when you call removeControl, the control is detached from the abstract control tree (so there are no references to that control from the tree), but it retains all its properties (for ex. errors), so clearing parent seems inconsistent from that perspective. Given the fact that it would also be a breaking change, I'm thinking if we should instead just extend the setParent to accept null as a possible argument, so the parent can be cleared if it's needed in a particular use-case. We will also keep your original ticket open to collect more feedback from the community to see if there are some use-cases where clearing parent after detaching a control is a blocker. What do you think?

Thank you.

[AndrewKushnir]

Closing this PR for now as this change may cause some inconsistencies with other APIs (see #29580 (comment)). I think it'd be great to create a new PR to extend the setParent function to accept null, given that the _parent field on the AbstractControl class now supports null (after #32671). Thank you.

[angular-automatic-lock-bot]

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.}