[WIP] feat(forms): add generic typings to AbstractControl

[Toxicable]

Please check if the PR fulfills these requirements

• The commit message follows our guidelines
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

What is the current behavior?
#13721 Currently there is no generic type support for AbstractControl
Therefore AbstractControl#value will always return a value with type: any

What is the new behavior?
Provides type support for AbstractControl#value and related fields/methods

For example:

const control = new FormControl('Toxicable');
const value = control.value;
const changes = control.valueChanges;

In this example control will be control: FormControl<string>
value will be value: string
changes will be changes: Observable<string>

Does this PR introduce a breaking change? (check one with "x")

[ x ] Yes
[ ] No

The breaking change is the TS2.3 requirement for default generics
With default generics it means we are able to make these changes without breaking the API surface
For example:

const control = new FormControl();
const value = control.value;

Here control will be control: FormControl<any>
value will be value: any
Which is the same behavior as the API currently provides

However. If someone is misusing the API like below, then it will indeed break their code

const control = new FormControl('Toxicable');
control.value.toFixed(3); //the user might be using the string as a number

This will throw with the TS compiler since it will now infer the type and know that control.value is a string

closes #13721

cc @kara

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

[googlebot]

CLAs look good, thanks!

[Toxicable]

The error in Travis is that one of the e2e tests hasn't been upgraded to TS2.3, that's why it's expecting a , on <T = any>

[Toxicable]

Im open for suggestions on how to default formState to null a bit nicer than what I got going on here

[desfero]

@Toxicable maybe we can add default value (null) to formState?

[Toxicable]

I did attempt to make it default to null but was unable to satisfy the TS compiler

[Toxicable]

Using this for testing the generics to make sure they can infer the correct things and will remove before done. If there's any specific behavior that isn't already tested then i'll add a specific test for it.

[Toxicable]

Since the signature now allows nulls we should do a proper null check here

[devoto13]

While I'm really looking forward for this PR to be merged, it will require Angular to stop supporting TS <2.3. So it may be considered a breaking change and require a major version bump... On the other hand updating from 2.1 and 2.2 shouldn't require changes in the application, so hopefully we can get it in the next minor.

[Toxicable]

@devoto13 Ahh right, I overlooked the need to upgrade to TS 2.3. Which means this is indeed a breaking change since people on TS 2.1 which Angular does currently support would have a broken build with this PR. However, it's possible that Angular will require TS2.3 in other areas by v5 which would be the next suitable point to merge this into

[desfero]

Maybe we could add generic default for AbstractControl#get method? Currenly it always return AbstractControl.

[Toxicable]

@desfero I havn't been able to make types that allows the lookups of AbstractControl#get for what type of control should be returned but once I solve the issue currently blocking this PR I will continue investigating how we might make this more ergonomic

[desfero]

@Toxicable maybe we can add default value (null) to formState?

[desfero]

@Toxicable is it some kind of good practice to have space between value and ![name]

[Toxicable]

The formatting isn't my choosing, it's as per Angular's Formatting build scripts, however in this case I don't think it's updated to handle variable![prop].

[desfero]

@Toxicable yes, you are right, we cannot lookup in the type, but programmer could explicitly specify that 'name' is for e.g. MyCustomFormControl, please have a look at #16769

[Toxicable]

@desfero oh right, totally overlooked giving it it's own generic, feels pretty janky but I don't think there's another option currently, ill add that in and will continue to look at how it might be improved

[Toxicable]

Blocked on microsoft/TypeScript#16229 for the time being

[googlebot]

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

[googlebot]

CLAs look good, thanks!

[Toxicable]

I just had another look at this and I don't think it's possible on the current Forms API.
There is an issue where if initialize a FormControl to null with strictNullChecks enabled then the generic will be inferred as null which means that it could cause breaking changes which was what we were aiming to avoid here.
Example here:

//inferred as: control: FormControl<null>
var control = FormControl(null);

//err: string != null
control.setValue('Toxicable');

So I'm closing this since I don't think it's possible to not cause breaking changes on the current Forms API

[xnnkmd]

@Toxicable What about changing default generics type to T = object ? Would that help?

[Toxicable]

@xnnkmd nope it would have the same effect

[Toxicable]

Here's an example of the breaking change
https://www.typescriptlang.org/play/#src=class%20FormControl%3CT%3Dany%3E%7B%0D%0A%20%20%20%20constructor(public%20value%3A%20T)%20%7B%20%7D%0D%0A%7D%0D%0Avar%20c%20%3D%20new%20FormControl(null)%0D%0Ac.value%20%3D%20'Toxicable'

You have to enable strictNullChecks by clicking Options > strictNullChecks

[xnnkmd]

Brilliant that you shared a simple playground to discuss this... I played a bit around with it.... What about this change instead:
https://www.typescriptlang.org/play/#src=class%20FormControl%3CT%3Dany%3E%7B%0D%0A%20%20%20%20constructor(public%20value%3A%20T)%20%7B%20%7D%0D%0A%7D%0D%0Avar%20c%20%3D%20new%20FormControl(null)%0D%0Ac.value%20%3D%20'Toxicable#src=type%20TDefault%20%3D%20any%20%7C%20null%3B%0D%0A%0D%0Aclass%20FormControl%3CT%20extends%20Object%20%3D%20any%3E%7B%0D%0A%20%20%20%20constructor(public%20value%3A%20T%20%7C%20null)%20%7B%20%7D%0D%0A%7D%0D%0Avar%20c%20%3D%20new%20FormControl(null)%0D%0Ac.value%20%3D%20'Toxicable'%3B

[xnnkmd]

@Toxicable Sorry, the link does not work. Here is the suggested code:

class FormControl<T extends Object = object>{
    constructor(public value: T | null) { }
}
var c = new FormControl(null)
c.value = 'Toxicable';

[xnnkmd]

@Toxicable This alternative also works for your example (need more test cases to be sure what works best and to check it does work in all cases):

class FormControl<T extends Object = object | number | string | symbol | boolean>{
    constructor(public value: T | null) { }
}
var c = new FormControl('x')
c.value = 'Toxicable';

[Toxicable]

Superseded by #20040

[angular-automatic-lock-bot]

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