Skip to content

Experimental: Strictly typed reactive forms #38406

Closed
sonukapoor wants to merge 13 commits intoangular:masterfrom
sonukapoor:strictly_types_reactive_forms
Closed

Experimental: Strictly typed reactive forms #38406
sonukapoor wants to merge 13 commits intoangular:masterfrom
sonukapoor:strictly_types_reactive_forms

Conversation

@sonukapoor
Copy link
Contributor

@sonukapoor sonukapoor commented Aug 11, 2020

Based on the original work from #37389

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Feature

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

@sonukapoor sonukapoor added area: forms forms: Controls API Issues related to AbstractControl, FormControl, FormGroup, FormArray. labels Aug 11, 2020
@ngbot ngbot bot modified the milestone: needsTriage Aug 11, 2020
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Aug 11, 2020

Presubmit.

@angular angular deleted a comment from googlebot Aug 11, 2020
destus90
destus90 reviewed Aug 11, 2020
View reviewed changes
@sonukapoor sonukapoor force-pushed the strictly_types_reactive_forms branch from 40607b1 to 50a99bf Compare August 11, 2020 10:57
@sonukapoor sonukapoor force-pushed the strictly_types_reactive_forms branch 2 times, most recently from 42eb11c to a4abb47 Compare August 11, 2020 11:17
@sonukapoor sonukapoor changed the title Experimental and test only - Strictly types reactive forms Experimental and test only - Strictly typed reactive forms Aug 11, 2020
@googlebot

This comment has been minimized.

Sign in to view
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Aug 11, 2020 •
edited
Loading

Hi,

Quick update after running tests in Google's codebase: this change is breaking a portion of targets.

I performed initial investigation and it looks like a lot of targets are failing with a similar error and we also have this scenario in tests:

const c = new FormControl(null, {validators});
c.setValue('value');

TypeScript infers the generic type (T) as null, thus the setValue call argument with the string type produces an error. In the tests this is solved by adding the <string> generic type, i.e.:

const c = new FormControl<string>(null, {validators});
c.setValue('value');

It'd be great to investigate if adding a generic type can be avoided in this case (without loosing type safety) - it should reduce the number of failing targets and simplify the migration for users.

Thank you.

@angular angular deleted a comment from googlebot Aug 11, 2020
@angular angular deleted a comment from googlebot Aug 11, 2020
@googlebot

This comment has been minimized.

Sign in to view
@sonukapoor sonukapoor force-pushed the strictly_types_reactive_forms branch 2 times, most recently from b0cfde2 to 623004d Compare August 21, 2020 12:30
KostyaTretyak and others added 12 commits March 31, 2021 18:26
@KostyaTretyak @AndrewKushnir
fix(forms): added hints for test form controls
f5ef3c8 
After introduce form control generics, fixed breaking changes:
- form group/array control need type hint like this `FormGroup<type>` sometimes
@KostyaTretyak @AndrewKushnir
fix(forms): fixed abstractControl.get(), formGroup.value and formArra…
ba68c12 
…y.value

- added generic for abstractControl.get()
- removed null as allowed value for formGroup/formArray value
@KostyaTretyak @AndrewKushnir
refactor(forms): renamed FormState to FormControlState
5a92ae1 
Because the FormState type is actually intended for FormControl constructor,
FormState is renamed to FormControlState.
@sonukapoor @AndrewKushnir
fixup! feat(forms): make reactive forms strongly typed
3306c82
@sonukapoor @AndrewKushnir
fixup! feat(forms): make reactive forms strongly typed
1fc6e4d
@sonukapoor @AndrewKushnir
fixup! feat(forms): make reactive forms strongly typed
c51506a
@sonukapoor @AndrewKushnir
fixup! feat(forms): make reactive forms strongly typed
631e496
@sonukapoor @AndrewKushnir
fixup! feat(forms): make reactive forms strongly typed
1747c37
@sonukapoor @AndrewKushnir
fixup! feat(forms): make reactive forms strongly typed
38a9d3f
@sonukapoor @AndrewKushnir
fixup! feat(forms): make reactive forms strongly typed
8e995a4
@AndrewKushnir
fixup! feat(forms): make reactive forms strongly typed
df52aa8
@AndrewKushnir
fixup! feat(forms): make reactive forms strongly typed
6bb0e5e
@AndrewKushnir AndrewKushnir force-pushed the strictly_types_reactive_forms branch from d8035b4 to 6bb0e5e Compare April 1, 2021 01:56
@google-cla
Copy link

google-cla bot commented Apr 1, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Apr 1, 2021
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Apr 1, 2021
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Apr 1, 2021

Presubmit.

@KostyaTretyak KostyaTretyak mentioned this pull request Jun 19, 2021
Closed
ydmitry
ydmitry reviewed Jun 29, 2021
View reviewed changes
packages/forms/test/value_accessor_integration_spec.ts
const input = fixture.debugElement.query(By.css('input'));
form.valueChanges.subscribe({
next: (value) => {
next: (value: any) => {
Copy link

@ydmitry ydmitry Jun 29, 2021

Choose a reason for hiding this comment

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

unknown instead of any could be better here

@dylhunn dylhunn mentioned this pull request Oct 14, 2021
Closed
@atscott
Copy link
Contributor

atscott commented Dec 16, 2021

Closing since this is being actively worked on by @dylhunn as part of the typed forms project (#43834, #44513)

@atscott atscott closed this Dec 16, 2021
@angular-automatic-lock-bot
Copy link

angular-automatic-lock-bot bot commented Jan 16, 2022

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 Jan 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JoostK JoostK JoostK left review comments

@AndrewKushnir AndrewKushnir Awaiting requested review from AndrewKushnir

+5 more reviewers

@ydmitry ydmitry ydmitry left review comments

@KostyaTretyak KostyaTretyak KostyaTretyak left review comments

@wiesnery wiesnery wiesnery left review comments

@destus90 destus90 destus90 left review comments

@leo6104 leo6104 leo6104 requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area: forms cla: yes forms: Controls API Issues related to AbstractControl, FormControl, FormGroup, FormArray.

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

10 participants

@sonukapoor @AndrewKushnir @googlebot @JoostK @atscott @ydmitry @KostyaTretyak @leo6104 @wiesnery @destus90