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.

Sign up for GitHub

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

feature(forms): Improve typings for (Async)Validators #48679

Closed
wants to merge 1 commit into from
Closed

feature(forms): Improve typings for (Async)Validators #48679

wants to merge 1 commit into from

Conversation

JeanMeche
Copy link
Member

@JeanMeche JeanMeche commented Jan 10, 2023 •
edited

With this commit, AsyncValidatorFn cannot be passed as ValidatorFn anymore in FormControl.

To exclude Promises, excluding object with then (see thenables), to exclude Observables I chose to exclude object with the key subscribe.

fixes: #48676

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature

Does this PR introduce a breaking change?

  • Yes (kinda)
  • No

The only breaking change should be misuse reports by the compiler of AsyncValidatorFn as ValidatorsFn in FormControl constructors.

@JeanMeche JeanMeche force-pushed the feature/form-control-typings-async-validators branch 2 times, most recently from 79a2100 to 37c6732 Compare January 10, 2023 16:13
@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Jan 10, 2023
@JeanMeche JeanMeche marked this pull request as ready for review January 11, 2023 16:57
@pullapprove pullapprove bot requested a review from dylhunn January 11, 2023 16:57
@JeanMeche JeanMeche changed the title feature(forms): Improve typings form (async)Validators feature(forms): Improve typings for (Async)Validators Jan 11, 2023
@dylhunn dylhunn added action: presubmit The PR is in need of a google3 presubmit action: global presubmit The PR is in need of a google3 global presubmit labels Jan 12, 2023
@ngbot ngbot bot added this to the Backlog milestone Jan 19, 2023
@dylhunn dylhunn added target: minor This PR is targeted for the next minor release and removed action: presubmit The PR is in need of a google3 presubmit labels Jan 24, 2023
dylhunn
dylhunn approved these changes Jan 24, 2023
View reviewed changes
Copy link
Contributor

@dylhunn dylhunn left a comment

Choose a reason for hiding this comment

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

reviewed-for: public-api

@pullapprove pullapprove bot requested a review from alxhub January 24, 2023 10:41
@pullapprove pullapprove bot requested a review from atscott January 24, 2023 11:24
@dylhunn dylhunn added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 24, 2023
@JeanMeche JeanMeche force-pushed the feature/form-control-typings-async-validators branch from 37c6732 to ee390e8 Compare January 24, 2023 13:44
atscott
atscott approved these changes Jan 24, 2023
View reviewed changes
Copy link
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

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

reviewed-for: public-api

@dylhunn dylhunn added action: presubmit The PR is in need of a google3 presubmit and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: review The PR is still awaiting reviews from at least one requested reviewer action: global presubmit The PR is in need of a google3 global presubmit labels Jan 24, 2023
@JeanMeche JeanMeche force-pushed the feature/form-control-typings-async-validators branch from ee390e8 to dd4e413 Compare March 25, 2023 23:19
@JeanMeche
Copy link
Member Author

I just solved the conflicts, is anything preventing the PR from moving forward ?

@JeanMeche
feat(forms): Improve typings form (async)Validators
da189de 
With this commit, AsyncValidatorFn cannot be passed as ValidatorFn  anymore in FormControl.

fixes: angular#48676
@JeanMeche JeanMeche force-pushed the feature/form-control-typings-async-validators branch from dd4e413 to da189de Compare March 26, 2023 00:03
jessicajaniuk
jessicajaniuk approved these changes Mar 30, 2023
View reviewed changes
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

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

reviewed-for: public-api

@jessicajaniuk jessicajaniuk removed the request for review from alxhub March 30, 2023 15:47
@jessicajaniuk jessicajaniuk 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 Mar 30, 2023
@angular-robot
Copy link
Contributor

angular-robot bot commented Mar 30, 2023

This PR was merged into the repository by commit 07a1aa3.

@angular-robot angular-robot bot closed this in 07a1aa3 Mar 30, 2023
@JeanMeche JeanMeche deleted the feature/form-control-typings-async-validators branch March 30, 2023 21:15
@dylhunn dylhunn mentioned this pull request Apr 3, 2023
Closed
dylhunn added a commit to dylhunn/angular that referenced this pull request Apr 3, 2023
@dylhunn
fix(forms): Exclude ValidatorFn from the inferred ControlConfig type
e4fe1ad 
Previous PR angular#48679 introduced an extraordinarily subtle issue with FormBuilder type inference. FormBuilder is way too polymorphic, and I'm looking forward to improving/replacing it with something that has fewer argument shapes.

In particular, this typing issue applied only when the following are all true:
1. There is a FormBuilder shorthand call
2. It uses the `ControlConfig` shorthand notation (e.g. `[value, validator]`)
3. The value itself is an array, which goes into a `FormControl`
4. The validator is not static, e.g. it's not `Validators.required`, but something that returns a dynamic `ValidatorFn`

In particular, angular#4 was the reason the breakage slipped through my very exhaustive tests.

This fix adds the recently-narrowed `ValidatorFn` back into the list of types that are systematically excluded from the inferred value type of the form control.

C’est la vie, and hoping to simplify FormBuilder someday.
@dylhunn
Copy link
Contributor

dylhunn commented Apr 4, 2023 •
edited

Independent of the bug fixed in #49693, this caused legitimate breakage in g3 (passing AsyncValidatorFn where ValidatorFn is expected), which needs to be cleaned up. We'll have to reopen this and try again.

dylhunn added a commit to dylhunn/angular that referenced this pull request Apr 4, 2023
@dylhunn
refactor(forms): Add a test that uses ControlConfig with a non-static…
b18dd0c 
… validator.

Previously, this PR cleaned up a bug introduced by angular#48679. However, since that PR needed to be rolled back, this PR now just checks in the test, to prevent that issue from re-occurring in the future.
dylhunn added a commit that referenced this pull request Apr 5, 2023
@dylhunn
refactor(forms): Add a test that uses ControlConfig with a non-static…
136ffbc 
… validator. (#49693)

Previously, this PR cleaned up a bug introduced by #48679. However, since that PR needed to be rolled back, this PR now just checks in the test, to prevent that issue from re-occurring in the future.

PR Close #49693
@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 May 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dylhunn dylhunn dylhunn approved these changes

@atscott atscott atscott approved these changes

@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: forms detected: feature PR contains a feature commit target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

AsyncValidatorFn can be passed as ValidatorFn to FormControl without warning
4 participants
@JeanMeche @dylhunn @atscott @jessicajaniuk