fix(forms): ensure to emit status change events on subsequent value-update / validations by kgopi · Pull Request #20806 · angular/angular

kgopi · 2017-12-05T17:14:28Z

fix(forms): ensure to emit status change events on subsequent value-update / validations

RCA:

During initialization of FormControl, emitEvent is getting passed as false to updateValueAndValidity function, in turn the same value got passed to the _runAsyncValidator function. (sets status = 'PENDING') and asyncValidator will be invoked.
When we set the FormControl to the FormGroup via FormGroup.setcontrol, internally it is calling updateValueAndValidity (on FormGroup) function with emitEvent as undefined, it causes status change event to be emitted on FormGroup instance. (causes 'PENDING' status change event)
When the asyncValidator (of FormControl) got resolved, setErrors function is getting invoked by passing the lexical scoped variable emitEvent (false in this case).
As emitEvent value is false, susbsequent status change events are not getting notified !!

If I understand correctly, status-change-events needs to be notified on value update / validations.

Please correct me, If I am wrong.

Issue: #20424

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?

Issue Number: #20424

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

suspectpart · 2019-12-11T08:17:50Z

@alexeagle, @alxhub, @amcdnl, @AndrewKushnir, @andrewseguin, @angular-jira-bot, @angular-pullapprove-bot, @benlesh, @b-laporte, @bowenni, @bradlygreen, @brandonroberts, @chembu, @clydin, @crisbeto, @dennispbrown, @devversion, @evmar, @filipesilva, @Foxandxss, @gkalpak, @google-admin, @googlebot, @gregmagolan, @hansl, @hazmatdance, @IgorMinar, @jasonaden, @jbogarthyde, @jelbourn, @jenniferfell, @johnpapa, @josephperrott, @jtraband, @juliemr, @kapunahelewong, @kara, @kyliau, @LucasSloan, @manughub, @marcelgood, @marclaval, @matsko, @mgechev, @mhevery, @mmalerba, @mprobst, @Narretz, @ocombe, @petebacondarwin, @pkozlowski-opensource, @rkirov, @robwormald, @sjtrimble, @stephenfluin, @vikerman, @vivian-hu, @wardbell

Any chance we can merge this (see discussion in #20424)?

mprobst · 2019-12-11T09:25:59Z

@suspectpart: please don't spam.

suspectpart · 2019-12-11T12:36:21Z

@suspectpart: please don't spam.

I am sorry for spamming you, not the best way for sure. I just didn't know who in the list of reviewers is still active and could provide some feedback. My apologies.

JasonRammoray · 2020-01-07T03:26:04Z

Having in mind the fact that everyone is quite busy, one can't help but notice this is quite a severe issue especially in cases when a form control is being encapsulated within a container with *ngIf structural directive that changes its value depending on a context.
To prove it's true, here is a simple demo (Angular 8) proving the issue is still there.
It's been a while now since the fix has been prepared for more than two years.
What shall be done in order to merge it with the master branch?
Thanks.

petebacondarwin · 2020-01-08T14:34:19Z

Not sure what others think, but I would be tempted to make this shorter by using !!asyncValidator.

I'd prefer shorter version as well 👍

…pdate / validations RCA: During initialization of FormControl, `emitEvent` is getting passed as `false` to `updateValueAndValidity` function, inturn the same value got passed to the `_runAsyncValidator` function. (sets status = 'PENDING') and asyncValidator will be invoked. When we set the `FormControl` to the `FormGroup` via `FormGroup.setcontrol`, internally it is calling `updateValueAndValidity` (on FormGroup) function with `emitEvent` as undefined, it causes status change event to be emitted on `FormGroup` instance. (causes 'PENDING' status change event) When the asyncValidator (of FormControl) got resolved, `setErrors` function is getting invoked by passing the lexical scoped variable `emitEvent` (false in this case). As emitEvent value is `false`, susbsequent status change events are not getting notified !! If I understand correctly, status-change-events needs to be notified on value update / validations. Please correct me, If I am wrong. Issue: angular#20424

petebacondarwin · 2020-01-08T14:36:57Z

Rebased on master...

petebacondarwin · 2020-01-08T14:37:37Z

To land this we need a presubmit from a Googler, and a review from someone in the angular/fw-forms code owner group.

AndrewKushnir · 2020-01-09T00:44:29Z

Presubmit

AndrewKushnir · 2020-01-09T21:11:50Z

Hi @kgopi, after running google3 tests, it turned out that this change breaks a few targets and additional investigation is required to find the origin of these failures and perform the necessary adjustments (either in the changes in this PR or in google3 apps code). I added the "blocked" label for now to indicate that and we'll get back to this PR once v9 final is released. Thank you.

suspectpart · 2020-01-10T08:49:26Z

Hi @kgopi, after running google3 tests, it turned out that this change breaks a few targets and additional investigation is required to find the origin of these failures and perform the necessary adjustments (either in the changes in this PR or in google3 apps code). I added the "blocked" label for now to indicate that and we'll get back to this PR once v9 final is released. Thank you.

Can you give a rough projection when this is gonna happen? Is it more like a few weeks or a couple of months?

AndrewKushnir · 2020-01-10T20:39:50Z

Can you give a rough projection when this is gonna happen? Is it more like a few weeks or a couple of months?

Hi @suspectpart, I can not provide any ETA at this moment unfortunately, since ETA also depends on post v9 prioritization. I've added this ticket to "v10-candidates" milestone, so it will be reviewed and prioritized after v9 is released. Thank you.

JasonRammoray · 2020-02-21T17:28:26Z

I added the "blocked" label for now to indicate that and we'll get back to this PR once v9 final is released.

@AndrewKushnir, the 9th version has been rolled out recently.
Could you, please, shed some light on google3 tests failure investigation process?
Thanks.

estankovic · 2020-05-15T14:35:24Z

Any updates on this one? It have been a while since v9 rolled out

AndrewKushnir · 2020-06-15T19:50:32Z

+    this.updateValueAndValidity({
+      onlySelf: true,
+      emitEvent: asyncValidator ? true : false
+    });  // `asyncValidator` will cause status change, and that needs to be notified


I'd propose moving the comment above the corresponding line + use !!asyncValidator as Pete proposed, something like this (I've also updated a comment a bit):

this.updateValueAndValidity({ onlySelf: true, // If `asyncValidator` is present, it will trigger control status change from `PENDING` to `VALID` or `INVALID`. // The status should be broadcasted via the `statusChanges` observable, so we set `emitEvent` to `true` // to allow that during the control creation process. emitEvent: !!asyncValidator });

The same/similar comment can be added to other places as well (FormGroup and FormArray).

AndrewKushnir · 2020-06-15T20:06:17Z

        expect(logger).toEqual(['change!']);
      });

+      it('should only emit (`VALID`) status change event in absence of `FormControl.asyncValidator`',


Thanks for adding tests 👍

I think we should increase test coverage a bit more to handle additional cases, for example:

Standalone FormControl with async validator and both statusChanges and valueChanges subscriptions and log what the output is. We should have 2 tests here tp cover both VALID and INVALID results of the validation.

Standalone FormControl with both sync and async validators and see what happens in statusChanges and valueChanges subscriptions when sync validators return true/false and async validator returns true/false.

Combination of FormGroup and a child FormControl to test the behavior when we initialize such combination and:

async validator is present on a FormControl level

async validator is present on both FormGroup and a child FormControl level

Similar to the previous item, but cover the setControl case (this is what is partially implemented already).

Similar to the one above, the combination of FormArray and a child FormControl: initialization phase and a case with calling some API methods that might trigger status change (like pushing new controls to the array).

Combination of all classes: FormGroup, FormArray (as a child to the FormGroup) and FormControl (as a child to FormArray).

Once we have improved coverage, we can also see what the difference is in what we emit into the statusChanges and valueChanges now vs what the new set of events are. That should help better formalize the "breaking changes" note to let developers know which changes to expect and how to handle them (if needed).

Please let me know if any additional information is required.

Quick comment to the previous one: I think it'd also be great to have a test that would replace/remove a control (via setControl and/or other methods) which is running an async validator at this moment (i.e. in PENDING state), to see which events would be emitted in statusChanges.

Sorry, one more comment: it looks like PR #22575 might also be related to your PR, since it fixes the problem with async validation status, so you may see some incorrect results (described in #22575) in the statusChanges log until #22575 is accepted/merged.

AndrewKushnir · 2020-06-15T20:10:12Z

Hi @kgopi, sorry for the delay in answering.

I've re-ran presubmit and it looks like there is a couple places where this change triggering some failures. I will perform additional investigation and let you know.

I also had a look at the changes and posted additional comments, mostly related to proposed increase in test coverage. Please let me know if you have any questions/comments.

Thank you.

AndrewKushnir · 2020-07-16T01:27:04Z

Hi @kgopi,

Just want to give a quick update: the PR #22575 that I mentioned (with the fixes to the control status with async validators) has landed recently (so you should have more accurate control status in tests now). Also I've updated a couple apps in Google's codebase to be compatible with this change.

If you have a chance to perform additional updates based on the comments above (and add tests as I mentioned in #20806 (comment)), please let us know.

Thank you.

AndrewKushnir · 2020-08-28T00:05:57Z

Hi @kgopi, thanks for working on this issue and proposing the fix. FYI, similar change was made in #38354, so I'm closing this PR in favor of #38354. Thank you.

angular-automatic-lock-bot · 2020-09-28T16:21:44Z

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: yes label Dec 5, 2017

kgopi mentioned this pull request Dec 6, 2017

Pull request failed: continuous-integration/travis-ci/pr — The Travis CI build failed Swiip/generator-gulp-angular#1185

Closed

kgopi force-pushed the 20424 branch from 3354e4f to 90bf523 Compare December 6, 2017 09:01

IgorMinar added the area: forms label Dec 6, 2017

kara added the action: review The PR is still awaiting reviews from at least one requested reviewer label Apr 3, 2018

HBO2 approved these changes Jun 6, 2018

View reviewed changes

kara added the type: bug/fix label Sep 18, 2018

jasonaden added this to the needsTriage milestone Jan 29, 2019

dzonatan mentioned this pull request Dec 4, 2019

When replacing form control which has async validator the status from statusChange stays PENDING #20424

Closed

oyo-kgopi approved these changes Dec 11, 2019

View reviewed changes

JasonRammoray approved these changes Jan 7, 2020

View reviewed changes

petebacondarwin approved these changes Jan 8, 2020

View reviewed changes

petebacondarwin self-assigned this Jan 8, 2020

petebacondarwin force-pushed the 20424 branch from 90bf523 to 0722c28 Compare January 8, 2020 14:36

kgopi requested a review from a team January 8, 2020 14:36

petebacondarwin removed their assignment Jan 8, 2020

petebacondarwin added the action: presubmit The PR is in need of a google3 presubmit label Jan 8, 2020

AndrewKushnir added the state: blocked label Jan 9, 2020

AndrewKushnir modified the milestones: needsTriage, v10-candidates Jan 10, 2020

AndrewKushnir added forms: types and removed forms: types labels May 7, 2020

AndrewKushnir added the breaking changes label Jun 11, 2020

AndrewKushnir requested changes Jun 15, 2020

View reviewed changes

AndrewKushnir mentioned this pull request Jun 18, 2020

FormGroup & FormControl statusChanges are not emitted on creation #14542

Closed

StephenFluin added the hotlist: devrel label Jun 26, 2020

sonukapoor self-assigned this Jul 25, 2020

AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Aug 25, 2020

AndrewKushnir closed this Aug 28, 2020

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 28, 2020

Conversation

kgopi commented Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

suspectpart commented Dec 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mprobst commented Dec 11, 2019

Uh oh!

suspectpart commented Dec 11, 2019

Uh oh!

JasonRammoray commented Jan 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petebacondarwin Jan 8, 2020

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir Jan 9, 2020

Choose a reason for hiding this comment

Uh oh!

petebacondarwin commented Jan 8, 2020

Uh oh!

petebacondarwin commented Jan 8, 2020

Uh oh!

AndrewKushnir commented Jan 9, 2020

Uh oh!

AndrewKushnir commented Jan 9, 2020

Uh oh!

suspectpart commented Jan 10, 2020

Uh oh!

AndrewKushnir commented Jan 10, 2020

Uh oh!

JasonRammoray commented Feb 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estankovic commented May 15, 2020

Uh oh!

AndrewKushnir Jun 15, 2020

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir Jun 15, 2020

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir Jun 15, 2020

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir commented Jun 15, 2020

Uh oh!

AndrewKushnir commented Jul 16, 2020

Uh oh!

AndrewKushnir commented Aug 28, 2020

Uh oh!

angular-automatic-lock-bot Bot commented Sep 28, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

15 participants

kgopi commented Dec 5, 2017 •

edited

Loading

suspectpart commented Dec 11, 2019 •

edited

Loading

JasonRammoray commented Jan 7, 2020 •

edited

Loading

JasonRammoray commented Feb 21, 2020 •

edited

Loading