[WIP] feat(forms): add generic typings to AbstractControl#16828
[WIP] feat(forms): add generic typings to AbstractControl#16828Toxicable wants to merge 1 commit intoangular:masterfrom
Conversation
|
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. |
83399bc to
9a7f5d5
Compare
|
CLAs look good, thanks! |
ce1a6bf to
8bb157b
Compare
b545ef4 to
dec1f15
Compare
|
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 |
packages/forms/src/model.ts
Outdated
There was a problem hiding this comment.
Im open for suggestions on how to default formState to null a bit nicer than what I got going on here
There was a problem hiding this comment.
@Toxicable maybe we can add default value (null) to formState?
There was a problem hiding this comment.
I did attempt to make it default to null but was unable to satisfy the TS compiler
There was a problem hiding this comment.
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.
packages/forms/src/model.ts
Outdated
There was a problem hiding this comment.
Since the signature now allows nulls we should do a proper null check here
dec1f15 to
48df0b6
Compare
|
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 |
|
@desfero I havn't been able to make types that allows the lookups of |
packages/forms/src/model.ts
Outdated
There was a problem hiding this comment.
@Toxicable maybe we can add default value (null) to formState?
packages/forms/src/model.ts
Outdated
There was a problem hiding this comment.
@Toxicable is it some kind of good practice to have space between value and ![name]
There was a problem hiding this comment.
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].
|
@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 |
48df0b6 to
e6a7a6e
Compare
|
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 |
c03622f to
83beeae
Compare
|
CLAs look good, thanks! |
83beeae to
d97d84d
Compare
|
I just had another look at this and I don't think it's possible on the current Forms API. 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 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: |
|
@Toxicable Sorry, the link does not work. Here is the suggested code: |
|
@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): |
|
Superseded by #20040 |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Please check if the PR fulfills these requirements
What is the current behavior?
#13721 Currently there is no generic type support for
AbstractControlTherefore
AbstractControl#valuewill always return a value with type:anyWhat is the new behavior?
Provides type support for
AbstractControl#valueand related fields/methodsFor example:
In this example
controlwill becontrol: FormControl<string>valuewill bevalue: stringchangeswill bechanges: Observable<string>Does this PR introduce a breaking change? (check one with "x")
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:
Here control will be
control: FormControl<any>valuewill bevalue: anyWhich 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
This will throw with the TS compiler since it will now infer the type and know that
control.valueis a stringcloses #13721
cc @kara