feat(forms): adding typings to form components #32450

Thomas-Waldbillig · 2019-09-02T22:21:53Z

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

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?

currently there is no way to have typechecking for form elements
Issue Number: N/A

What is the new behavior?

creating FormStatus enum (reusable)
adding typing to AbstractControl (value, status, valueChanges, statusChanges)
adding typing to FormControl (constructor, setValue, patchValue, reset)
adding typing to FormGroup (constructor, registerControl, addControl, removeControl, setControl, contains, setValue, patchValue, reset, getRawValue)
adding typing to FormArray (constructor, at, push, insert, setControl, setValue, patchValue, reset, getRawValue)

PR Checklist

Does this PR introduce a breaking change?

Yes
No

Other information

Airblader · 2019-09-03T06:14:37Z

packages/forms/src/model.ts

   */
-  public readonly value: any;
+  public readonly value: T|null = null;


Isn't undefined also a valid value?

imho the value should match the type that was specified.
so, to reset it should use null and if the type is only composed of optional attributes you should pass an empty object.
or is there any special case where passing undefined might be important?

Ultimately that doesn't matter, if you do not allow undefined here when it was allowed before, this is a breaking change.

well if you don't specify a type T defaults to any which allows undefined which should pass, no?
so the current untyped code should not be affected

Ah, true, any | null will just collapse to any anyway.

The value should really be DeepPartial<T>. Even the docs say you only get the enabled controls of a FormGroup.
Because FormGroups can be nested, value can only contain a partial of each group.

Airblader · 2019-09-03T06:16:41Z

I understand that emoji in commit messages are somewhat modern, but Angular doesn't use those and since the commit messages are compiled into the changelog I would consider removing it.

trotyl · 2019-09-03T06:31:13Z

Partially duplicate of #20040

Thomas-Waldbillig · 2019-09-03T07:50:04Z

I understand that emoji in commit messages are somewhat modern, but Angular doesn't use those and since the commit messages are compiled into the changelog I would consider removing it.

the emoji comes from a small app that helps with commit messages (https://github.com/commitizen/cz-cli)
the emoji is linked to the type of commit and I find it quite practical when searching for a commit to locate it more easily

but I can get rid of it

Thomas-Waldbillig · 2019-09-03T07:57:29Z

Partially duplicate of #20040

I hadn't seen that one...
b.t.w. does anyone have some recommendations/doc on how to setup and test your environment when working on the angular code base?

- creating FormStatus enum (reusable) - adding typing to AbstractControl (value, status, valueChanges, statusChanges) - adding typing to FormControl (constructor, setValue, patchValue, reset) - adding typing to FormGroup (constructor, registerControl, addControl, removeControl, setControl, contains, setValue, patchValue, reset, getRawValue) - adding typing to FormArray (constructor, at, push, insert, setControl, setValue, patchValue, reset, getRawValue)

mhevery · 2019-09-03T22:42:08Z

packages/forms/src/model.ts

@@ -12,19 +12,26 @@ import {composeAsyncValidators, composeValidators} from './directives/shared';
 import {AsyncValidatorFn, ValidationErrors, ValidatorFn} from './directives/validators';
 import {toObservable} from './validators';

+export enum FormStatus {


Suggested change

export enum FormStatus {

export const enum FormStatus {

ngbot · 2019-09-03T22:43:30Z

I see that you just added the PR action: merge label, but the following checks are still failing:
     status "ci/angular: size" is failing
     status "ci/circleci: build-ivy-npm-packages" is failing
     status "ci/circleci: build-npm-packages" is failing
     status "ci/circleci: legacy-unit-tests-saucelabs" is failing
     status "ci/circleci: lint" is failing
     status "ci/circleci: test" is failing
     status "ci/circleci: test_ivy_aot" is failing
     status "google3" is pending
     missing required status "ci/circleci: publish_snapshot"

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

mhevery · 2019-09-04T18:38:53Z

@Waldbillig-Thomas can you try to get this PR green?

Thomas-Waldbillig · 2019-09-04T20:17:33Z

@Waldbillig-Thomas can you try to get this PR green?

i started to look into it, but I probably won't have time to continue until this weekend

I say that there was an error when you created a formcontrol with null as value and no type, typescript infers that the type T is null and not any, which causes problems...

and for the rest, I have to check what else is failing

b.t.w. I am having some problems running scripts from the package.json locally. does anyone know where I can find some more information about this? resp. Is there an easier way to check out stuff locally?

no0x9d

As much as I want type checking for Angular forms but your pull request contains breaking changes.
When using typed forms Angular must support all valid use cases.
For some cases it's difficult to find the correct type. In my oppinion values should be of type DeepPartial<T>. This is not correct for FormControls but is needed for FormControls or else not all operations are possible.

no0x9d · 2019-11-05T20:46:08Z

packages/forms/src/model.ts

   */
-  public readonly value: any;
+  public readonly value: T|null = null;


The value should really be DeepPartial<T>. Even the docs say you only get the enabled controls of a FormGroup.
Because FormGroups can be nested, value can only contain a partial of each group.

no0x9d · 2019-11-05T20:59:59Z

packages/forms/src/model.ts

@@ -297,7 +303,7 @@ export abstract class AbstractControl {
   * the UI or programmatically.
   */
  // TODO(issue/24571): remove '!'.
-  public readonly valueChanges !: Observable<any>;
+  public readonly valueChanges !: Observable<T|null>;


Same as with value the type of valueChanges should be a DeepPartial<T> instead only type T.

no0x9d · 2019-11-05T21:02:39Z

packages/forms/src/model.ts

@@ -1014,7 +1020,7 @@ export class FormControl extends AbstractControl {
  *
  */
  constructor(
-      formState: any = null,
+      formState: T|null = null,


FormControl also accepts an object with the type {value: T, disabled: boolean}

Maybe extract to a type:

type FormState<T> = T | { value: T, disabled: boolean };

This can be used in more places as well

no0x9d · 2019-11-05T21:06:00Z

packages/forms/src/model.ts

@@ -1072,13 +1078,13 @@ export class FormControl extends AbstractControl {
   *
   * @see `setValue` for options
   */
-  patchValue(value: any, options: {
+  patchValue(value: T extends Object ? Partial<T> : T, options: {


Why can value be Partial<T>? For FormControl setValue and patchValue are the same and should have the same type. Here we can remove required properties of T. This should not be possible.

no0x9d · 2019-11-05T21:14:18Z

packages/forms/src/model.ts

@@ -1424,7 +1430,7 @@ export class FormGroup extends AbstractControl {
   * The configuration options are passed to the {@link AbstractControl#updateValueAndValidity
   * updateValueAndValidity} method.
   */
-  patchValue(value: {[key: string]: any}, options: {onlySelf?: boolean, emitEvent?: boolean} = {}):
+  patchValue(value: Partial<T>, options: {onlySelf?: boolean, emitEvent?: boolean} = {}):


value should be of type DeepPartial<T>. With only Partial<T> even the example for reactive forms in the official docs can not be typed correctly, because the method updateProfile() uses a deep partial on a FormGroup (see docs).

no0x9d · 2019-11-05T21:18:21Z

packages/forms/src/model.ts

@@ -1837,7 +1843,7 @@ export class FormArray extends AbstractControl {
   * The configuration options are passed to the {@link AbstractControl#updateValueAndValidity
   * updateValueAndValidity} method.
   */
-  patchValue(value: any[], options: {onlySelf?: boolean, emitEvent?: boolean} = {}): void {
+  patchValue(value: T[], options: {onlySelf?: boolean, emitEvent?: boolean} = {}): void {


This one is tricky. If the FormArray contains only FormControls the type of T[] is correct. But if the FormArray contains FormGroups the correct type is Partial<T>, for nested FormGroups DeepPartial<T>.
The type of T[] is not backwards compatible.

no0x9d · 2019-11-05T21:22:50Z

packages/forms/src/model.ts

@@ -1680,8 +1686,8 @@ export class FormArray extends AbstractControl {
  *
  */
  constructor(
-      public controls: AbstractControl[],
-      validatorOrOpts?: ValidatorFn|ValidatorFn[]|AbstractControlOptions|null,
+    public controls: AbstractControl<T[]>[],


I think the type should be AbstractControl<T>[]

pauldraper · 2019-12-04T17:05:15Z

When using typed forms Angular must support all valid use cases.

That is a very, very odd statement for a framework that chose TypeScript.

A typed language never, ever attempts to support all legal untyped operations.

'5' - 3 is a legal JS/untyped operation, but illegal TS/typed operation. There are ways to workaround this by altering the program +'5' - 3, or by descending into untyped mode <any>'5' - 3.

Between #16828/#20040 (2 years), #31963 (4 months), and #32450 (2 months), it'd be great to see a solution for TypeScript programming that deemed acceptable.

Airblader · 2019-12-04T17:23:10Z

@no0x9d didn't say that the Forms API need to support any shenanigan you can throw against it, but that it needs to support valid, existing use-cases, such as the different inputs these functions can take, dynamically adding and removing controls etc. And I definitely agree with that. Types are supposed to help us, not break APIs by not allowing us to pass valid input.

pauldraper · 2019-12-10T18:24:14Z

Types are supposed to help us, not break APIs by not allowing us to pass valid input.

And my point is that TypeScript "broke" JavaScript APIs, as do all types.

[{name: 'one'}, {name: 'two'}].sort((a, b) => (b.name < a.name) - (a.name < b.name));

That is perfectly valid JavaScript.

It is not valid TypeScript.

If you want a type-checker to prove correct uses of types, it's not unreasonable to require different programming patterns than you might be without a type checker.

Better types are of course better. But hopefully it will not take Angular another 2 years to arrive at those for something as exotic as forms.

mr1upmachine · 2020-03-03T13:34:39Z

@Waldbillig-Thomas do you have plans on updating this PR any further? If not, I can branch off this and begin to try and address the issues above

Thomas-Waldbillig · 2020-03-15T19:33:48Z

Currently I do not really have the time. You can close it for now and if/when I have more time again I will create a new pull request Thomas

…

On Tue, 3 Mar 2020, 14:34 Sean Hale, ***@***.***> wrote: @Waldbillig-Thomas <https://github.com/Waldbillig-Thomas> do you have plans on updating this PR any further? If not, I can branch off this and begin to try and address the issues above — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#32450?email_source=notifications&email_token=AAEK2UOE5ESQCOKCWWR7DSTRFUBPFA5CNFSM4ITAQIDKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENTP5TQ#issuecomment-593952462>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEK2ULV35VNOV4QUN6VKVTRFUBPFANCNFSM4ITAQIDA> .

AndrewKushnir · 2020-12-09T06:56:48Z

Closing in favor of more recent #38906 and #38406. Thank you.

angular-automatic-lock-bot · 2021-01-09T16:56:44Z

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.}

Thomas-Waldbillig requested a review from a team as a code owner September 2, 2019 22:21

googlebot added the cla: yes label Sep 2, 2019

Airblader reviewed Sep 3, 2019

View reviewed changes

Thomas-Waldbillig added 3 commits September 3, 2019 22:54

style: removing lint errors

4cb84e1

fix: making patchValue handle primitives types correctly

e9a3e5b

Thomas-Waldbillig changed the title ~~feat(forms): 🎸 adding typings to form components~~ feat(forms): adding typings to form components Sep 3, 2019

fix: formArray => adjusting constructor typing

54f61bf

mhevery approved these changes Sep 3, 2019

View reviewed changes

mhevery added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release labels Sep 3, 2019

mhevery added the area: core Issues related to the framework runtime label Sep 3, 2019

ngbot bot added this to the needsTriage milestone Sep 3, 2019

mhevery self-assigned this Sep 3, 2019

kara added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Sep 12, 2019

pmccloghrylaing mentioned this pull request Sep 13, 2019

feat(forms): add types to Forms - both controls and builder #32655

Closed

14 tasks

no0x9d suggested changes Nov 5, 2019

View reviewed changes

mhevery assigned AndrewKushnir and unassigned mhevery Nov 19, 2020

pullapprove bot requested a review from AndrewKushnir November 19, 2020 18:08

AndrewKushnir closed this Dec 9, 2020

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(forms): adding typings to form components #32450

feat(forms): adding typings to form components #32450

Thomas-Waldbillig commented Sep 2, 2019

Airblader Sep 3, 2019

Thomas-Waldbillig Sep 3, 2019

Airblader Sep 3, 2019

Thomas-Waldbillig Sep 3, 2019

Airblader Sep 3, 2019

no0x9d Nov 5, 2019

Airblader commented Sep 3, 2019

trotyl commented Sep 3, 2019

Thomas-Waldbillig commented Sep 3, 2019 •

edited

Thomas-Waldbillig commented Sep 3, 2019

mhevery Sep 3, 2019

ngbot bot commented Sep 3, 2019

mhevery commented Sep 4, 2019

Thomas-Waldbillig commented Sep 4, 2019 •

edited

no0x9d left a comment

no0x9d Nov 5, 2019

no0x9d Nov 5, 2019

no0x9d Nov 5, 2019

gioragutt Dec 27, 2019

no0x9d Nov 5, 2019

no0x9d Nov 5, 2019

no0x9d Nov 5, 2019

no0x9d Nov 5, 2019

pauldraper commented Dec 4, 2019 •

edited

Airblader commented Dec 4, 2019 •

edited

pauldraper commented Dec 10, 2019 •

edited

mr1upmachine commented Mar 3, 2020

Thomas-Waldbillig commented Mar 15, 2020 via email

AndrewKushnir commented Dec 9, 2020

angular-automatic-lock-bot bot commented Jan 9, 2021

feat(forms): adding typings to form components #32450

feat(forms): adding typings to form components #32450

Conversation

Thomas-Waldbillig commented Sep 2, 2019

PR Type

What is the current behavior?

What is the new behavior?

PR Checklist

Does this PR introduce a breaking change?

Other information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Airblader commented Sep 3, 2019

trotyl commented Sep 3, 2019

Thomas-Waldbillig commented Sep 3, 2019 • edited

Thomas-Waldbillig commented Sep 3, 2019

Choose a reason for hiding this comment

ngbot bot commented Sep 3, 2019

mhevery commented Sep 4, 2019

Thomas-Waldbillig commented Sep 4, 2019 • edited

no0x9d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pauldraper commented Dec 4, 2019 • edited

Airblader commented Dec 4, 2019 • edited

pauldraper commented Dec 10, 2019 • edited

mr1upmachine commented Mar 3, 2020

Thomas-Waldbillig commented Mar 15, 2020 via email

AndrewKushnir commented Dec 9, 2020

angular-automatic-lock-bot bot commented Jan 9, 2021

Thomas-Waldbillig commented Sep 3, 2019 •

edited

Thomas-Waldbillig commented Sep 4, 2019 •

edited

pauldraper commented Dec 4, 2019 •

edited

Airblader commented Dec 4, 2019 •

edited

pauldraper commented Dec 10, 2019 •

edited