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

feat(forms): Implement strict types for the Angular Forms package. #43834

Closed
wants to merge 1 commit into from
Closed

feat(forms): Implement strict types for the Angular Forms package. #43834

wants to merge 1 commit into from

Conversation

dylhunn
Copy link
Contributor

@dylhunn dylhunn commented Oct 14, 2021 •
edited

This PR strongly types the forms package by adding generics to AbstractControl classes as well as FormBuilder. This makes forms type-safe and null-safe, for both controls and values.

The design uses a "control-types" approach. In other words, the type parameter on FormGroup is an object containing controls, and the type parameter on FormArray is a control.

Issue: #13721

@google-cla google-cla bot added the cla: yes label Oct 14, 2021
@dylhunn dylhunn added area: forms forms: Controls API Issues related to AbstractControl, FormControl, FormGroup, FormArray. labels Oct 14, 2021
@ngbot ngbot bot modified the milestone: Backlog Oct 14, 2021
@dylhunn dylhunn added action: discuss breaking changes design complexity: major needs: discussion Indicates than an issue is an open discussion risk: high risky Identifies a PR that has a level of risk to merging state: WIP target: major This PR is targeted for the next major release labels Oct 14, 2021
@dylhunn dylhunn modified the milestones: Backlog, v14-candidates Oct 14, 2021
@dylhunn dylhunn mentioned this pull request Oct 14, 2021
Closed
@NetanelBasal
Copy link

NetanelBasal commented Oct 14, 2021 •
edited

There are a lot of challenges in this area due to the flexibility of the API. As far as I can tell, I've written a typed version that reaches the maximum we can without breaking the API. Feel free to look at the source code:

https://github.com/ngneat/reactive-forms

Furthermore, it would be helpful to add specs for the types in this case. You can see an example here:

https://github.com/ngneat/reactive-forms/blob/master/libs/reactive-forms/src/lib/form-group.spec.ts#L333

@dylhunn
Copy link
Contributor Author

dylhunn commented Oct 14, 2021 •
edited

I've written a typed version that reaches the maximum we can without breaking the API

Wonderful, somehow I wasn't aware of this. I'll be sure to have a look.

@dylhunn dylhunn force-pushed the typed-forms-default branch 2 times, most recently from 2d42212 to d99a19e Compare October 15, 2021 01:45
@dylhunn dylhunn force-pushed the typed-forms-default branch 2 times, most recently from 9cfa76c to 5cc693e Compare November 3, 2021 06:54
AndrewKushnir
AndrewKushnir reviewed Nov 4, 2021
View reviewed changes
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.

FYI, just sharing first batch of comments, will publish more comments tomorrow.

packages/forms/src/directives/validators.ts Outdated Show resolved Hide resolved
packages/forms/src/form_builder.ts Show resolved Hide resolved
packages/forms/src/form_builder.ts Outdated Show resolved Hide resolved
packages/forms/src/form_builder.ts Outdated Show resolved Hide resolved
packages/forms/src/form_builder.ts Outdated Show resolved Hide resolved
packages/forms/src/form_builder.ts Outdated Show resolved Hide resolved
packages/forms/src/model.ts Outdated Show resolved Hide resolved
This was referenced Nov 4, 2021
Closed
Closed
@mary-poppins
Copy link

You can preview f161700 at https://pr43834-f161700.ngbuilds.io/.

@angular angular deleted a comment from mary-poppins Apr 8, 2022
@dylhunn dylhunn force-pushed the typed-forms-default branch 4 times, most recently from bcce3cf to 7959624 Compare April 8, 2022 23:05
@dylhunn dylhunn force-pushed the typed-forms-default branch 3 times, most recently from 034ddce to 52f9aaa Compare April 11, 2022 23:58
@dylhunn
Copy link
Contributor Author

dylhunn commented Apr 11, 2022 •
edited

TGP is green, with two final fixup CLs patched: one, two

merge-assistance: merge separately, Tuesday morning

@dylhunn dylhunn added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: presubmit The PR is in need of a google3 presubmit labels Apr 11, 2022
@ngbot ngbot bot added the action: merge The PR is ready for merge by the caretaker label Apr 11, 2022
@dylhunn
feat(forms): Implement strict types for the Angular Forms package.
15d86e1 
This PR strongly types the forms package by adding generics to AbstractControl classes as well as FormBuilder. This makes forms type-safe and null-safe, for both controls and values.

The design uses a "control-types" approach. In other words, the type parameter on FormGroup is an object containing controls, and the type parameter on FormArray is an array of controls.

Special thanks to Alex Rickabaugh and Andrew Kushnir for co-design & implementation, to Sonu Kapoor and Netanel Basal for illustrative prior art, and to Cédric Exbrayat for extensive testing and validation.

BREAKING CHANGE: Forms classes accept a generic.

Forms model classes now accept a generic type parameter. Untyped versions of these classes are available to opt-out of the new, stricter behavior.
@dylhunn dylhunn added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Apr 12, 2022
@ngbot ngbot bot added the action: merge The PR is ready for merge by the caretaker label Apr 12, 2022
@jessicajaniuk
Copy link
Contributor

This PR was merged into the repository by commit 89d2991.

@depyronick
Copy link

it's been 5+ years of wait but worth it. thanks everybody!

@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 Jun 10, 2022
@dylhunn dylhunn deleted the typed-forms-default branch October 17, 2022 13:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sonukapoor sonukapoor sonukapoor left review comments

@alfaproject alfaproject alfaproject left review comments

@JoostK JoostK JoostK left review comments

@cexbrayat cexbrayat cexbrayat left review comments

@SchnWalter SchnWalter SchnWalter left review comments

@itotallyrock itotallyrock itotallyrock left review comments

@atscott atscott atscott approved these changes

@alxhub alxhub alxhub approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

@learn2grid learn2grid learn2grid approved these changes

@jelbourn jelbourn Awaiting requested review from jelbourn

@jessicajaniuk jessicajaniuk Awaiting requested review from jessicajaniuk

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: forms breaking changes cla: yes cross-cutting: types design complexity: major forms: Controls API Issues related to AbstractControl, FormControl, FormGroup, FormArray. forms: strictly typed merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note risk: high target: major This PR is targeted for the next major release
Projects
None yet
Milestone
v14-candidates
Development

Successfully merging this pull request may close these issues.

Reactive forms are not strongly typed