Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(forms): ensure to emit statusChanges on subsequent value update… #38354

Closed

Conversation

sonukapoor
Copy link
Contributor

@sonukapoor sonukapoor commented Aug 5, 2020

… / validations

This commit ensures that the updateValueAndValidity method takes the
asyncValidator into consideration to emit on the statusChanges observables.
This is necessary so that any subsequent changes are emitted properly to any
subscribers.

BREAKING CHANGE:

Previously if FormControl, FormGroup and FormArray class instances had async validators
defined at initialization time, the status change event was not emitted once async validator
completed. After this change, the status event is emitted into the statusChanges observable.
If your code relies on the old behaviour, you can filter/ignore this additional status change
event.

Closes #20424
Closes #14542

PR Checklist

Please check if your PR fulfills the following requirements:

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: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding tests @sonukapoor 👍

I've left a few comments (most of them are applicable to many tests), please have a look when you get a chance. Once the feedback is addressed, I will have another look.

Thank you.

packages/forms/test/form_group_spec.ts Outdated Show resolved Hide resolved
packages/forms/test/form_group_spec.ts Outdated Show resolved Hide resolved
packages/forms/test/form_group_spec.ts Outdated Show resolved Hide resolved
packages/forms/test/form_group_spec.ts Outdated Show resolved Hide resolved
packages/forms/test/form_group_spec.ts Outdated Show resolved Hide resolved
@sonukapoor
Copy link
Contributor Author

@AndrewKushnir All changes are done. The PR is ready for the second review.

@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Aug 11, 2020

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sonukapoor, thanks for additional changes. I've left a few more comments, please have a look when you get a chance. Thank you.

packages/forms/test/form_group_spec.ts Show resolved Hide resolved
packages/forms/test/form_group_spec.ts Outdated Show resolved Hide resolved
packages/forms/test/form_group_spec.ts Outdated Show resolved Hide resolved
packages/forms/test/form_group_spec.ts Show resolved Hide resolved
packages/forms/test/form_group_spec.ts Outdated Show resolved Hide resolved
@sonukapoor
Copy link
Contributor Author

@AndrewKushnir It's ready for another review. Thanks

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating tests @sonukapoor, great work 👍

I've a few more comments, they are mostly around comments/test descriptions, so I think we should move this PR to the "Ready for Review" state and perform a final round of review.

Thank you.

packages/forms/test/form_group_spec.ts Outdated Show resolved Hide resolved
packages/forms/test/form_group_spec.ts Show resolved Hide resolved
packages/forms/test/form_group_spec.ts Outdated Show resolved Hide resolved
packages/forms/test/form_group_spec.ts Outdated Show resolved Hide resolved
packages/forms/test/form_group_spec.ts Outdated Show resolved Hide resolved
packages/forms/test/form_group_spec.ts Outdated Show resolved Hide resolved
packages/forms/test/form_group_spec.ts Outdated Show resolved Hide resolved
packages/forms/test/form_group_spec.ts Outdated Show resolved Hide resolved
@sonukapoor sonukapoor force-pushed the fix/emit-status-change-events branch from 85f007d to 3a84683 Compare August 13, 2020 12:32
@sonukapoor sonukapoor marked this pull request as ready for review August 13, 2020 12:49
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating descriptions in tests, this is super helpful 👍

I've just left a comment on the last test, everything else looks great!

Since this is a change in behavior, could you please include a draft of the breaking changes note into the commit description (we can review and update it)?

packages/forms/test/form_group_spec.ts Show resolved Hide resolved
@AndrewKushnir AndrewKushnir added target: major This PR is targeted for the next major release breaking changes labels Aug 15, 2020
@Neck26638

This comment has been minimized.

@Neck26638

This comment has been minimized.

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks Sonu 👍

@sonukapoor sonukapoor force-pushed the fix/emit-status-change-events branch 2 times, most recently from a245226 to ba5f4dc Compare August 25, 2020 23:14
@AndrewKushnir
Copy link
Contributor

Global Presubmit #2.

@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Aug 27, 2020
…/validations

This commit ensures that the `updateValueAndValidity` method takes the
`asyncValidator` into consideration to emit on the `statusChanges` observables.
This is necessary so that any subsequent changes are emitted properly to any
subscribers.

Closes angular#20424
Closes angular#14542

BREAKING CHANGE:

Previously if FormControl, FormGroup and FormArray class instances had async validators
defined at initialization time, the status change event was not emitted once async validator
completed. After this change the status event is emitted into the `statusChanges` observable.
If your code relies on the old behavior, you can filter/ignore this additional status change
event.
@sonukapoor sonukapoor force-pushed the fix/emit-status-change-events branch from ba5f4dc to 9d59b7d Compare August 27, 2020 10:14
@AndrewKushnir
Copy link
Contributor

Note to Caretaker: the global presubmit went well (only pre-existing failures), however since it's technically a breaking change, it might make sense to merge it and sync as a separate change (in case a rollback is needed). Thank you.

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Aug 28, 2020
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
…/validations (angular#38354)

This commit ensures that the `updateValueAndValidity` method takes the
`asyncValidator` into consideration to emit on the `statusChanges` observables.
This is necessary so that any subsequent changes are emitted properly to any
subscribers.

Closes angular#20424
Closes angular#14542

BREAKING CHANGE:

Previously if FormControl, FormGroup and FormArray class instances had async validators
defined at initialization time, the status change event was not emitted once async validator
completed. After this change the status event is emitted into the `statusChanges` observable.
If your code relies on the old behavior, you can filter/ignore this additional status change
event.

PR Close angular#38354
@sonukapoor sonukapoor deleted the fix/emit-status-change-events branch September 8, 2020 18:50
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: forms breaking changes cla: yes forms: validators risk: medium target: major This PR is targeted for the next major release
Projects
None yet
5 participants