Skip to content

fix(forms): improve types of directive constructor arguments #38944

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

Closed

Conversation

AndrewKushnir
Copy link
Contributor

Prior to this change, the validators and asyncValidators fields of a few Forms directives
were typed as any[]. This commit updates the types and makes them consistent for all directives
in the Forms package.

BREAKING CHANGE:

Directives in Forms package used to have any[] as a type of validators and asyncValidators
arguments in constructors. Now these arguments are properly typed, so if your code relies on
directive constructor types it may require some updates to improve type safety.

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Yes, see description above
  • No

@ngbot ngbot bot added this to the needsTriage milestone Sep 23, 2020
@AndrewKushnir
Copy link
Contributor Author

AndrewKushnir commented Sep 23, 2020

Initial presubmit + Global TAP run (internal-only links).

@AndrewKushnir AndrewKushnir force-pushed the forms_directive_ctr_types branch from af5d0d0 to 88fd6d4 Compare September 29, 2020 00:46
@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Sep 29, 2020
@AndrewKushnir AndrewKushnir marked this pull request as ready for review September 29, 2020 00:54
AndrewKushnir added a commit to AndrewKushnir/ngcc-validation that referenced this pull request Sep 30, 2020
This PR is created for testing purposes only to check angular/angular#38944 PR with listed applications.
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewed-for: public-api

@AndrewKushnir
Copy link
Contributor Author

Just a quick update: this change has limited impact in Google's codebase (only 1 target was affected) and the tests went well in the ngcc-validation repo. This is still a breaking change, but its impact on user's code is minor.

Copy link
Contributor

@petebacondarwin petebacondarwin 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

Copy link
Contributor

@IgorMinar IgorMinar 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 verifying the impact of this breaking change. I think most of the breakages will be within the application (as opposed to 3rd party library) code, so developers should be able to resolve them during an upgrade along with other breakages caused by improved TypeScript compiler checks.

LGTM!

Reviewed-for: public-api

@AndrewKushnir AndrewKushnir removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 6, 2020
Prior to this change, the `validators` and `asyncValidators` fields of a few Forms directives
were typed as `any[]`. This commit updates the types and makes them consistent for all directives
in the Forms package.

BREAKING CHANGE:

Directives in the `@angular/forms` package used to have `any[]` as a type of `validators` and
`asyncValidators` arguments in constructors. Now these arguments are properly typed, so if your
code relies on directive constructor types it may require some updates to improve type safety.
@AndrewKushnir AndrewKushnir force-pushed the forms_directive_ctr_types branch from 88fd6d4 to 4711696 Compare October 6, 2020 16:33
@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker risk: low labels Oct 6, 2020
@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 Nov 6, 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 risk: low target: major This PR is targeted for the next major release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants