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

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.

CLAs look good, thanks!

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>

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.

@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

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

@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

@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

@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

Blocked on microsoft/TypeScript#16229 for the time being

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.

CLAs look good, thanks!

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

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

@xnnkmd nope it would have the same effect

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

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

@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';

@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';

Superseded by #20040

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