fix(forms): prevent event emission on enable/disable when emitEvent … #21018

teh-skrud · 2017-12-14T16:52:05Z

…is false (#12366)

Previously, the emitEvent flag was only checked when emitting on the current control. Thus, if the control was part of a hierarchy, events were emitted on the parent and the childrens. This fixes the issue by properly passing the emitEvent flag to both parent and childrens.

Fixes #12366

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?

[x] 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?

See #12366
For instance, if we have a FormGroup > FormArray > FormControl hierarchy and we disable the FormArray with emitEvent: false, the events (valueChanges, statusChanges) are still emitted on both the FormGroup and the FormControl.

What is the new behavior?

The emitEvent flag is properly passed to the parent and the childrens. Thus, in our previous example, not events are emitted on neither the FormGroup nor the FormControl

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

googlebot · 2017-12-14T16:52:09Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

teh-skrud · 2017-12-15T07:50:51Z

I signed it!

googlebot · 2017-12-15T07:50:54Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

googlebot · 2017-12-15T08:23:47Z

CLAs look good, thanks!

kara

Looks good, just needs the tests to be fixed.

@teh-skrud Can you ping me when this is ready for review again?

kara · 2018-01-11T23:42:43Z

packages/forms/test/form_array_spec.ts

+
+        it('should not emit status change events when emitEvent = false', () => {
+          c.valueChanges.subscribe(() => logger.push('control'));
+          a.valueChanges.subscribe(() => logger.push('array'));


Looks like these should be statusChanges subscriptions, no?

kara · 2018-01-11T23:43:06Z

packages/forms/test/form_control_spec.ts

+        });
+
+        it('should not emit status change events when emitEvent = false', () => {
+          c.valueChanges.subscribe(() => logger.push('control'));


Looks like this should be statusChanges

googlebot · 2018-01-12T08:28:29Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

googlebot · 2018-01-12T08:34:44Z

CLAs look good, thanks!

…s false (angular#12366) Previously, the emitEvent flag was only checked when emitting on the current control. Thus, if the control was part of a hierarchy, events were emitted on the parent and the childrens. This fixes the issue by properly passing the emitEvent flag to both parent and childrens. Fixes angular#12366

teh-skrud · 2018-01-12T10:36:37Z

@kara : Thx for the review. I've fixed the tests and rebased the whole on the current master.

kara

LGTM

kara · 2018-02-07T01:32:23Z

presubmit

…s false (#12366) (#21018) Previously, the emitEvent flag was only checked when emitting on the current control. Thus, if the control was part of a hierarchy, events were emitted on the parent and the childrens. This fixes the issue by properly passing the emitEvent flag to both parent and childrens. Fixes #12366 PR Close #21018

…s false (angular#12366) (angular#21018) Previously, the emitEvent flag was only checked when emitting on the current control. Thus, if the control was part of a hierarchy, events were emitted on the parent and the childrens. This fixes the issue by properly passing the emitEvent flag to both parent and childrens. Fixes angular#12366 PR Close angular#21018

angular-automatic-lock-bot · 2019-09-13T06:52:55Z

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

googlebot added the cla: no label Dec 14, 2017

teh-skrud force-pushed the fix-12366 branch from 050de0a to a0b3d71 Compare December 15, 2017 08:23

googlebot added cla: yes and removed cla: no labels Dec 15, 2017

teh-skrud force-pushed the fix-12366 branch from a0b3d71 to 55909be Compare December 15, 2017 10:37

teh-skrud closed this Dec 15, 2017

teh-skrud reopened this Dec 15, 2017

teh-skrud closed this Dec 18, 2017

teh-skrud reopened this Dec 18, 2017

IgorMinar added the area: forms label Dec 22, 2017

kara suggested changes Jan 11, 2018

View reviewed changes

kara added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 11, 2018

kara assigned teh-skrud Jan 11, 2018

teh-skrud force-pushed the fix-12366 branch from 55909be to 13aa25e Compare January 12, 2018 08:28

googlebot added cla: no and removed cla: yes labels Jan 12, 2018

teh-skrud force-pushed the fix-12366 branch from 13aa25e to 830e1ed Compare January 12, 2018 08:34

googlebot added cla: yes and removed cla: no labels Jan 12, 2018

teh-skrud force-pushed the fix-12366 branch from 830e1ed to 222022b Compare January 12, 2018 09:47

kara approved these changes Feb 7, 2018

View reviewed changes

kara added the target: patch This PR is targeted for the next patch release label Feb 7, 2018

kara removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 7, 2018

kara assigned kara and unassigned teh-skrud Feb 7, 2018

kara added the action: merge The PR is ready for merge by the caretaker label Feb 7, 2018

kara removed their assignment Feb 7, 2018

mhevery closed this in 0bcfae7 Feb 7, 2018

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(forms): prevent event emission on enable/disable when emitEvent … #21018

fix(forms): prevent event emission on enable/disable when emitEvent … #21018

teh-skrud commented Dec 14, 2017 •

edited

Loading

googlebot commented Dec 14, 2017

teh-skrud commented Dec 15, 2017

googlebot commented Dec 15, 2017

googlebot commented Dec 15, 2017

kara left a comment •

edited

Loading

kara Jan 11, 2018

kara Jan 11, 2018

googlebot commented Jan 12, 2018

googlebot commented Jan 12, 2018

teh-skrud commented Jan 12, 2018

kara left a comment

kara commented Feb 7, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

fix(forms): prevent event emission on enable/disable when emitEvent … #21018

fix(forms): prevent event emission on enable/disable when emitEvent … #21018

Conversation

teh-skrud commented Dec 14, 2017 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

googlebot commented Dec 14, 2017

teh-skrud commented Dec 15, 2017

googlebot commented Dec 15, 2017

googlebot commented Dec 15, 2017

kara left a comment • edited Loading

Choose a reason for hiding this comment

kara Jan 11, 2018

Choose a reason for hiding this comment

kara Jan 11, 2018

Choose a reason for hiding this comment

googlebot commented Jan 12, 2018

googlebot commented Jan 12, 2018

teh-skrud commented Jan 12, 2018

kara left a comment

Choose a reason for hiding this comment

kara commented Feb 7, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

teh-skrud commented Dec 14, 2017 •

edited

Loading

kara left a comment •

edited

Loading