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

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.

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

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

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.

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.

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.

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