feat(forms): add emitEvent option for AbstractControl-based class methods

[MikeJerred]

This commit adds an optional argument to the methods addControl
and removeControl in the FormGroup class. This argument can be
used to prevent an event from being emitted.

PR Close #29662

PR Checklist

Please check if your PR fulfills the following requirements:

• 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

What kind of change does this PR introduce?

• 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?

Issue Number: #29662

What is the new behavior?

An argument can now be specified to prevent an event from being emitted.

Does this PR introduce a breaking change?

• Yes
• No

Other information

[MikeJerred]

I don't know why the build is failing, it was working in PR #29664 which had exactly the same code changes

[JulianFarhi]

Do we have some news about PR review?

[AndrewKushnir]

Hi @MikeJerred, sorry we didn't have a chance to review/reply earlier.

As I mentioned in #20439 (comment), we'd like to consider adding emitEvent config to the FormGroup and FormArray classes. This PR covers most of the functions already (except the FormGroup.setControl one) and has good test coverage, so I'm wondering if you might be interested in rebasing this PR and adding the emitEvent support to the FormGroup.setControl method as well? If you don't have time to look into that now, I can help with rebasing and extending FormGroup.setControl API.

Thank you.

[MikeJerred]

@AndrewKushnir I have rebased this PR and added the changes to FormGroup.setControl. The tests are failing but as far as I can tell the failure is not related to my changes - perhaps you could help with debugging that?

[AndrewKushnir]

Hi @MikeJerred, thanks for updating the PR!

The CI failure was a flake, restarting that CI job resolved the issue.

I will review the changes within the next couple days and get back with comments/feedback.

Thank you.

[AndrewKushnir]

@MikeJerred thanks for updating the PR! The changes look good, I've just left a few comments related to docs and tests. Please let me know if you have any questions and/or need any help. Thank you.

[AndrewKushnir]

Thanks for additional changes @MikeJerred 👍

I've left a few more comments mostly around docs to rephrase them a bit to be aligned with the function itself.

Just as a thought for a future refactoring (later in a separate PR), it'd be great to make description of the opts argument more generic (do not name the action specifically, but just refer to it as the "current action"), so that it's easier to maintain. The docs may look like:

   * @param opts Allows to specify whether events should be emitted once the current 
   * operation is completed.
   * * `emitEvent`: When true or not supplied (the default), both `statusChanges` and 
   * `valueChanges` events are emitted with the latest status and value. When false, no 
   * events are emitted.

We'd probably need to revisit exiting functions as well and update opts argument docs in a similar way. But this is definitely outside of the scope of this PR (I'll create a ticket once this PR lands).

Thank you.

[AndrewKushnir]

@MikeJerred thanks for applying the changes.

I noticed that you've changed the implementation for options from using default value to making the second argument optional. I think these options are close, but the default value approach is more consistent with existing APIs. Could you please update it?

Also I want to share the next steps:

• since this PR changes public API surface, more people from the public-api group would review the PR and provide feedback
• address the feedback from reviewers
• run tests in Google's codebase to make sure there are no breakages (we'll take care of that)
• add "BREAKING CHANGES" section to the commit message and describe that custom classes that extend AbstractControl-based ones and override existing methods might need some updates

Thank you.

[petebacondarwin]

Great. Thanks @MikeJerred

[petebacondarwin]

Reviewed-for: public-api

[AndrewKushnir]

Presubmit #2 + Global Presubmit #2 (internal-only links).

[mary-poppins]

You can preview 21dad98 at https://pr31031-21dad98.ngbuilds.io/.
You can preview 2e305bd at https://pr31031-2e305bd.ngbuilds.io/.
You can preview b0ea8b2 at https://pr31031-b0ea8b2.ngbuilds.io/.
You can preview 07cce2a at https://pr31031-07cce2a.ngbuilds.io/.
You can preview 58dc91b at https://pr31031-58dc91b.ngbuilds.io/.
You can preview af4753f at https://pr31031-af4753f.ngbuilds.io/.
You can preview aa533c1 at https://pr31031-aa533c1.ngbuilds.io/.
You can preview 5a24fc6 at https://pr31031-5a24fc6.ngbuilds.io/.
You can preview 6072db2 at https://pr31031-6072db2.ngbuilds.io/.
You can preview 7218be5 at https://pr31031-7218be5.ngbuilds.io/.
You can preview e02d862 at https://pr31031-e02d862.ngbuilds.io/.
You can preview 473821f at https://pr31031-473821f.ngbuilds.io/.

[AndrewKushnir]

@MikeJerred quick update: tests in Google's codebase went well (after minor updates to classes that extend FormGroup and FormArray). We are very close to merging this change and the last step would be to squash all commits into one and add the "BREAKING CHANGE" note to the commit body, so the commit message may look like this:

feat(forms): add `emitEvent` option for AbstractControl-based class methods

This commit adds the `emitEvent` option to the following FromArray and FormGroup methods:

* FormGroup.addControl
* FormGroup.removeControl
* FormGroup.setControl
* FormArray.push
* FormArray.insert
* FormArray.removeAt
* FormArray.setControl
* FormArray.clear

This option can be used to prevent an event from being emitted when adding or removing controls.

BREAKING CHANGE:

The `emitEvent` option was added to the following `FromArray` and `FormGroup` methods:

* FormGroup.addControl
* FormGroup.removeControl
* FormGroup.setControl
* FormArray.push
* FormArray.insert
* FormArray.removeAt
* FormArray.setControl
* FormArray.clear

If your app has custom classes that extend `FromArray` or `FormGroup` classes and override the 
above-mentioned methods, you may need to update your implementation to take new options into
account and make sure that overrides are compatible from types perspective.

Closes #29662.

Please let us know once it's ready, we'll do final checks and add this PR to the merge queue.

Thank you.

[MikeJerred]

@AndrewKushnir I have squashed the commits into one and adjusted the message.

[rafaelss95]

@MikeJerred there are some typos in your commit: FromArray should be FormArray.

[AndrewKushnir]

FYI, started a new presubmit in Google's codebase (internal-only link) to verify that there there are no places that might be broken by this change. Will keep this thread updated.

[AndrewKushnir]

FYI all affected tests in Google's codebase are successful, so I'm adding this PR to the merge queue.

@MikeJerred thanks for working on this PR and addressing the feedback! The change will be merged into the master branch and should be available as a part of the next major release (v12) in a few weeks.

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