Skip to content

/ angular

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

Jump to bottom

feat(forms): add addValidators and hasValidators methods to AbstractControl #42838

Closed
wants to merge 1 commit into from
Closed

feat(forms): add addValidators and hasValidators methods to AbstractControl #42838

wants to merge 1 commit into from
+313 −24

Conversation

@dylhunn
Copy link
Contributor

@dylhunn dylhunn commented Jul 13, 2021 •
edited

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: #13461

It is currently not possible to modify the validator set on a FormControl, or check for a particular validator's presence.

What is the new behavior?

Add getValidators and hasValidators methods (for both sync and async) to the FormControl API, enabling vaioud use cases in a longstanding issue.

Several new functionalities are possible with this change: the most requested is that callers can now check whether a control has a required validator. Other uses include incrementally changing the validators set without doing an expensive operation to reset all validators.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Closes #13461.
@google-cla google-cla bot added the cla: yes label Jul 13, 2021
@splincode
splincode reviewed Jul 13, 2021
View changes
packages/forms/src/model.ts Show resolved Hide resolved
@dylhunn dylhunn force-pushed the dylhunn:validators-methods branch from 699892a to 951376f Jul 13, 2021
@AndrewKushnir AndrewKushnir changed the title feat(forms): add getValidators and hasValidators methods feat(forms): add addValidators and hasValidators methods Jul 13, 2021
@AndrewKushnir
AndrewKushnir reviewed Jul 14, 2021
View changes
packages/forms/src/model.ts Outdated Show resolved Hide resolved
packages/forms/src/model.ts Outdated Show resolved Hide resolved
packages/forms/src/model.ts Outdated Show resolved Hide resolved
packages/forms/src/model.ts Outdated Show resolved Hide resolved
packages/forms/src/model.ts Outdated Show resolved Hide resolved
@dylhunn dylhunn marked this pull request as ready for review Jul 15, 2021
@dylhunn dylhunn requested a review from AndrewKushnir Jul 15, 2021
@dylhunn dylhunn force-pushed the dylhunn:validators-methods branch from 951376f to 3be0c01 Jul 15, 2021
@dylhunn dylhunn changed the title feat(forms): add addValidators and hasValidators methods feat(forms): add addValidators and hasValidators methods to AbstractControl Jul 15, 2021
@dylhunn dylhunn force-pushed the dylhunn:validators-methods branch 2 times, most recently from e2edcae to f531579 Jul 15, 2021
@AndrewKushnir
AndrewKushnir reviewed Jul 15, 2021
View changes
packages/forms/src/model.ts Outdated Show resolved Hide resolved
packages/forms/src/model.ts Outdated Show resolved Hide resolved
packages/forms/src/model.ts Outdated Show resolved Hide resolved
packages/forms/src/model.ts Outdated Show resolved Hide resolved
@ngbot ngbot bot modified the milestone: Backlog Jul 15, 2021
@dylhunn dylhunn force-pushed the dylhunn:validators-methods branch from f531579 to 6e055de Jul 15, 2021
@dylhunn dylhunn requested a review from AndrewKushnir Jul 15, 2021
@dylhunn dylhunn force-pushed the dylhunn:validators-methods branch 4 times, most recently from 27986b3 to 1aa5c5b Jul 15, 2021
@AndrewKushnir
AndrewKushnir approved these changes Jul 16, 2021
View changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

@dylhunn thanks for addressing the feedback 👍
The changes look good, just added a few minor comments related to docs and tests.
packages/forms/src/model.ts Show resolved Hide resolved
packages/forms/src/model.ts Show resolved Hide resolved
packages/forms/src/model.ts Outdated Show resolved Hide resolved
packages/forms/src/validators.ts Outdated Show resolved Hide resolved
packages/forms/test/form_control_spec.ts Show resolved Hide resolved
packages/forms/test/form_control_spec.ts
c.addValidators(Validators.required);
c.addValidators(Validators.required);
expect(c.hasValidator(Validators.required)).toEqual(true);
Comment on lines +350 to +353

This comment has been minimized.

Sign in to view
@AndrewKushnir

AndrewKushnir Jul 16, 2021
Contributor

I think we'd also need a separate test to verify that adding the same validator twice (or more) doesn't actually add multiple copies.
packages/forms/test/form_control_spec.ts Show resolved Hide resolved
@pullapprove pullapprove bot requested review from alxhub, atscott and IgorMinar Jul 16, 2021
@dylhunn dylhunn requested review from petebacondarwin and removed request for jelbourn Jul 21, 2021
@dylhunn
Copy link
Contributor Author

@dylhunn dylhunn commented Jul 21, 2021 •
edited

LGTM for size changes, but you might want to consider removing the setValidators/setAsyncValidators methods since they are not strictly required (you can clear validators and readd them) and add a bit of JS. I recommend keeping the API surface as small as possible to reduce the long term maintenance/docs/payload size etc costs. The two methods don't make a huge difference in this case so I'll leave it up to you to consider the trade offs.

Reviewed-for: size-tracking

@IgorMinar Thanks for the comment! setValidators/setAsyncValidators are the two preexisting methods, so to remove them we'd have to deprecate and provide a migration schematic in v13, which seems out of scope for this PR. Let me know if I'm misunderstanding.

@dylhunn
Copy link
Contributor Author

@dylhunn dylhunn commented Jul 21, 2021 •
edited

@petebacondarwin Thanks for the comments!

There is a small typo in the commit message: vaioud. Also, personally I would limit the title of the commit to a shorter sentence, e.g.

Fixed!

Finally there is a corner case that I think is a bit broken. Consider:

c.addValidators([Validators.required, Validators.required]);

I went ahead and fixed this -- now, c.addValidators([Validators.required, Validators.required]) will only add one copy.

@dylhunn dylhunn force-pushed the dylhunn:validators-methods branch from a2991c8 to 38627b2 Jul 21, 2021
@dylhunn dylhunn force-pushed the dylhunn:validators-methods branch 3 times, most recently from acdaa81 to d5470b7 Jul 21, 2021
@dev054
Copy link

@dev054 dev054 commented Jul 21, 2021

@dylhunn it'd be nice to mention the addtion of removeValidators in the commit message too.

@AndrewKushnir
AndrewKushnir reviewed Jul 21, 2021
View changes
packages/forms/src/validators.ts Outdated Show resolved Hide resolved
goldens/size-tracking/integration-payloads.json Outdated
@@ -49,7 +49,7 @@
"master": {
"uncompressed": {
"runtime-es2015": 1150,
"main-es2015": 162346,
"main-es2015": 163066,

This comment has been minimized.

Sign in to view
@AndrewKushnir

AndrewKushnir Jul 21, 2021
Contributor

I think this number should be slightly different now (since some common code was moved to helper functions). Could you please reset it back and rerun the CI to see if payload size is over the threshold?
packages/forms/src/model.ts Show resolved Hide resolved
packages/forms/src/model.ts Show resolved Hide resolved
packages/forms/src/validators.ts Show resolved Hide resolved
packages/forms/src/model.ts Outdated Show resolved Hide resolved
@IgorMinar
Copy link
Member

@IgorMinar IgorMinar commented Jul 21, 2021

@IgorMinar Thanks for the comment! setValidators/setAsyncValidators are the two preexisting methods, so to remove them we'd have to deprecate and provide a migration schematic in v13, which seems out of scope for this PR. Let me know if I'm misunderstanding.

@dylhunn ah, I see. I misread the diff. Removing an already existing API comes with a non-negligible cost, so I recommend against it in this case. Let's keep it as is in the PR. thank you

@dylhunn dylhunn force-pushed the dylhunn:validators-methods branch 2 times, most recently from 0ce5b9d to cd145c2 Jul 21, 2021
@dylhunn
feat(forms): add hasValidators, addValidators, and removeValidators m…
7acb62e 
…ethods (for both sync and async)

Several new functionalities are possible with this change: the most requested is that callers can now check whether a control has a required validator. Other uses include incrementally changing the validators set without doing an expensive operation to reset all validators.

Closes #13461.
@dylhunn dylhunn force-pushed the dylhunn:validators-methods branch from cd145c2 to 7acb62e Jul 21, 2021
@dylhunn dylhunn removed the request for review from petebacondarwin Jul 21, 2021
@dylhunn dylhunn closed this in 1d9d026 Jul 22, 2021
@dylhunn dylhunn mentioned this pull request Jul 22, 2021
Closed
@mmalerba mmalerba mentioned this pull request Jul 22, 2021
Open
@khodabandelu
Copy link

@khodabandelu khodabandelu commented Jul 27, 2021

hy guys ,
this feature support custom validator or only common validator that exist in angular?
@dylhunn

@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Jul 27, 2021

this feature support custom validator or only common validator that exist in angular?

@khodabandelu extra APIs added in this PR support any validators (built-in as well as custom ones).

@e-oz
Copy link

@e-oz e-oz commented Jul 29, 2021

Delicious! Thank you very much for this 👍

@e-oz
Copy link

@e-oz e-oz commented Aug 5, 2021 •
edited

Is it possible to backport it to 12.x branch? Please!

@JoostK
Copy link
Member

@JoostK JoostK commented Aug 5, 2021

@e-oz This is available in 12.2 and will not be backported further as it's a feature.

@e-oz
Copy link

@e-oz e-oz commented Aug 5, 2021

@JoostK then it's just not mentioned in the changelog :) Thank you for the good news 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@splincode splincode

@indraraj26 indraraj26

@dev054 dev054

@AndrewKushnir AndrewKushnir

@sg-gdarnell sg-gdarnell

@petebacondarwin petebacondarwin

@IgorMinar IgorMinar

@atscott atscott

@alxhub alxhub

@jessicajaniuk jessicajaniuk

Assignees
No one assigned
Labels
action: merge cla: yes comp: forms feature forms: validators target: minor
Projects
None yet
Milestone
Backlog
Linked issues

Successfully merging this pull request may close these issues.

FormControl.setValidators...why is there no getValidators?