Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(forms): add generic typings to AbstractControl #20040

Open
wants to merge 1 commit into
base: master
from

Conversation

@Toxicable
Copy link
Contributor

Toxicable commented Oct 30, 2017

PR Checklist

Please check if your PR fulfills the following 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)

PR Type

What kind of change does this PR introduce?

[x] Feature

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?

[ ] Yes
[x] No

Duplicate of #16828 since I kinda messed up that branch
closes #13721

@googlebot googlebot added the cla: yes label Oct 30, 2017
@Toxicable Toxicable force-pushed the Toxicable:abstract-form-control branch 4 times, most recently from e1f0154 to 16a256b Oct 30, 2017
@Toxicable Toxicable force-pushed the Toxicable:abstract-form-control branch 6 times, most recently from f98de13 to 4420bc9 Oct 31, 2017
@Trophalaxeur

This comment has been minimized.

Copy link

Trophalaxeur commented Nov 16, 2017

Stop me if i'm wrong but,
if i have an object with this interface :

interface MyList {
    name: string[],
    other: number,
}

I need to construct a form like this (without types) :

myform = new FormGroup({
    name: new FormArray([]),
    other: new FormControl(0)
)

If i use typed forms, the property 'name' need to be FormArray
And myform need to be FormGroup
Like,

myform = new FormGroup<MyList>({
    name: new FormArray<string>([]),
    other: new FormControl<number>(0)
)

But, if FormGroup contructor take
controls: {[key in keyof T]: AbstractControl<T[key]>}

Property 'name' will be of type AbstractControl<string[]>. And this is not compatible with FormArray<string>

@Toxicable

This comment has been minimized.

Copy link
Contributor Author

Toxicable commented Nov 16, 2017

@Trophalaxeur AbstractControl is the base class of all of the other 3 classes.
With FormArray you specify the type of a single item, but the type passed to AbstractControl is an array of that single type as you can see on the signature
FormArray<T = any> extends AbstractControl<T[]>

Therefore AbstractControl<string[]> and FormArray<string> are the same
here's a demo showing this to be true
https://stackblitz.com/edit/angular-gitter-zofzrc?file=app%2Ftyped-controls.ts

@c69

This comment has been minimized.

Copy link

c69 commented Dec 12, 2017

@Toxicable great job on implementing this.
But i see the PR is stuck at (final) documentation phase - are you accepting extra PRs, and if so maybe i can help ?

@Toxicable

This comment has been minimized.

Copy link
Contributor Author

Toxicable commented Dec 12, 2017

@c69 untill there is a final API on this there is no need to write docs since the API could change before it lands.

@jotatoledo

This comment has been minimized.

Copy link
Contributor

jotatoledo commented Dec 20, 2017

Nice work 👏

@Toxicable Toxicable force-pushed the Toxicable:abstract-form-control branch 4 times, most recently from d50cad9 to 7f4444a Dec 29, 2017
@Toxicable Toxicable force-pushed the Toxicable:abstract-form-control branch from 7f4444a to ecc26ae Dec 29, 2017
@ironstine

This comment has been minimized.

Copy link

ironstine commented Jan 8, 2018

Nice one @Toxicable!

@indrek-rajamets

This comment has been minimized.

Copy link

indrek-rajamets commented Jan 29, 2018

Nice work, any hope on merging this soon?

@ngbot

This comment has been minimized.

Copy link

ngbot bot commented Jan 30, 2018

Hi @Toxicable! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

1 similar comment
@ngbot

This comment has been minimized.

Copy link

ngbot bot commented Jan 30, 2018

Hi @Toxicable! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@ntziolis

This comment has been minimized.

Copy link

ntziolis commented Feb 17, 2018

Any update on this? Happy to assist with documenting.

@c69

This comment has been minimized.

Copy link

c69 commented Mar 14, 2018

Does this have any chance to get into 6.0.0 ?

@@ -105,7 +105,7 @@ function isOptionsObj(
*
* @stable
*/
export abstract class AbstractControl {
export abstract class AbstractControl<T = any> {

This comment has been minimized.

Copy link
@yawaramin

yawaramin Mar 22, 2018

Nice–this looks backwards-compatible.

@@ -528,7 +531,7 @@ export abstract class AbstractControl {
/**
* Retrieves the top-level ancestor of this control.
*/
get root(): AbstractControl {
get root(): AbstractControl<any> {

This comment has been minimized.

Copy link
@yawaramin

yawaramin Mar 22, 2018

Can this getter be universally quantified by the type of the root ancestor? E.g., get root<AncestorT>(): AbstractControl<AncestorT> { ... }

@@ -196,6 +196,11 @@ import {of } from 'rxjs/observable/of';
expect(form.value).toEqual({parent: {'one': '', 'two': ''}});
});

it('should throw when passed null', () => {
const g = new FormGroup({'a': new FormControl()});
expect(() => g.setValue(null as any)).toThrowError(new RegExp('Must supply a value'));

This comment has been minimized.

Copy link
@yawaramin

yawaramin Mar 22, 2018

Why is the error message a regex?

Copy link

no0x9d left a comment

This is a good change while maintaining backwards compatibility.
On a side note: This PR does not close #13721, because it only updates AbstractControl and the subclasses and not FormBuilder. Following #22202 this should also be part of this issue.
I'm currently in the process to publish a project with strongly typed forms without backwards compatibility, because i think this brings a lot more type safety (https://github.com/no0x9d/ngx-strongly-typed-forms).
I nearly have a typed FormBuilder with all edge cases and can make a follow up PR, when this is merged.


/**
* Patches the value of the control. Abstract method (implemented in sub-classes).
*/
abstract patchValue(value: any, options?: Object): void;
abstract patchValue(value: T, options?: Object): void;

This comment has been minimized.

Copy link
@no0x9d

no0x9d Mar 26, 2018

patchValue allows you to set only a subset of all controls in a complex form group. The correct type should be the mapped type Partial<T>, or else there is no difference between setValue and patchValue.

@@ -712,7 +715,7 @@ export class FormControl extends AbstractControl {
_pendingChange: any;

constructor(
formState: any = null,
formState: T|null = null,

This comment has been minimized.

Copy link
@no0x9d

no0x9d Mar 26, 2018

The constructor of FormControl may also take a object {value: T, disabled: boolean}

@@ -504,7 +504,10 @@ export abstract class AbstractControl {
*
* * `this.form.get(['person', 'name']);`
*/
get(path: Array<string|number>|string): AbstractControl|null { return _find(this, path, '.'); }
get<ControlT extends keyof T = any>(path: ControlT|Array<string|number>|

This comment has been minimized.

Copy link
@no0x9d

no0x9d Mar 26, 2018

This method is a real pain, because in my opinion it's just for convenience and it's impossible to staticly type it at all.
FormControl never returns anything other than null, FormGroup only work with string keys and FormArray with numbers.
But if you want to go the extra mile you can do some typings for e.g. a path 5 levels deep with typescript tuples. This comes at the cost of more complex type definitions. I could do a pull request for this.

@LmKupke

This comment has been minimized.

Copy link

LmKupke commented Jun 20, 2018

Will this be merged for Angular 6 in a minor release?

@jotatoledo

This comment has been minimized.

Copy link
Contributor

jotatoledo commented Jul 6, 2018

Any updates on this?

@kara kara added the jira-sync label Aug 22, 2018
@kara kara self-assigned this Aug 22, 2018
@kara kara added the type: feature label Sep 18, 2018
@at-cuongtran

This comment has been minimized.

Copy link

at-cuongtran commented Nov 9, 2018

I really want this!!!! Can't wait to see this in action!!!!

@hisham

This comment has been minimized.

Copy link

hisham commented Nov 9, 2018

+1

@dannyskoog

This comment has been minimized.

Copy link
Contributor

dannyskoog commented Nov 9, 2018

I’m actually surprised that this hasn’t made its way into the framework yet :) Seems like a no-brainer that people want strongly typed functionality these days.

Anyhow - me and my team is waiting eagerly on this one

@maxime1992

This comment has been minimized.

Copy link

maxime1992 commented Nov 11, 2018

Yes Angular team is aware of that but mentioned during angular Connect 2018 that they are waiting for some TS bug to be fixed 🤷‍♂️

@sarunint

This comment has been minimized.

Copy link
Contributor

sarunint commented Nov 12, 2018

In case you wonder, it's blocked on microsoft/TypeScript#16229.

@dimavt

This comment has been minimized.

Copy link

dimavt commented Dec 20, 2018

Hello!
It is possible to resolve this with current TS version. Rewritten example from microsoft/TypeScript#16229:

class O<T=any> {
  constructor(public array: T[]) { }
}

type Unwrap<T> = T extends O<infer U> ? U : T

declare let val: O<number> | O<string>;
declare function f<V extends O<any>, T=Unwrap<V> >(x: V): T;

const res = f(val); // works

type returnType = typeof res // string | number

f(1); // Argument of type '1' is not assignable to parameter of type 'O<any>'.

playground link

This one also works.

type Unwrap<T> = T extends AbstractControl<infer U> ? U : never

class AbstractControl<T extends any = any> {
  value: T;
}
class FormGroup<T extends any> extends AbstractControl<T> {
  constructor(
    public controls: {[key in keyof T]: AbstractControl<T[key]>}
  ) {
    super()
  }
}
class FormArray<C extends AbstractControl<any> , T = Unwrap<C>> extends AbstractControl<T>{
  constructor(
    public controls: C[]
  ) {
    super()
  }
}
class FormControl<T extends any> extends AbstractControl<T>{
  constructor(value: T) {
    super()
    this.value = value
  }
}


let a = new FormArray([
  new FormGroup({'c2': new FormControl('v2'), 'c3': new FormControl('v3')}),
  new FormArray([new FormControl('v4'), new FormControl(5)])
]);

a.controls[0].value // string | number | { 'c2': string; 'c3': string; }
a.controls[1].value // string | number | { 'c2': string; 'c3': string; }

playground link

I can experiment a bit with typescript tuples to get
a.controls[0].value -> { 'c2': string; 'c3': string; }
and a.controls[1].value -> string | number if needed.

@jasonaden jasonaden added this to the needsTriage milestone Jan 29, 2019
@Lonli-Lokli

This comment has been minimized.

Copy link

Lonli-Lokli commented May 31, 2019

@IgorMinar
what are the priorities in Angular team? More powerful Typescript allows to create more type-safe scenarios, while Angular still focus in Ivy which will answer answer Life, the Universe and Everything question?

@criskrzysiu

This comment has been minimized.

Copy link

criskrzysiu commented Aug 23, 2019

Can't we just ask the guys who created https://github.com/no0x9d/ngx-strongly-typed-forms to allow to implement their way of strongly typing into angular? Of course there can be some improvements made thanks to new features of current TS but it is already great implementation with over 1000+ daily downloads on npm.

@trotyl trotyl mentioned this pull request Sep 3, 2019
3 of 14 tasks complete
@kara kara removed their assignment Sep 9, 2019
@pauldraper

This comment has been minimized.

Copy link

pauldraper commented Dec 4, 2019

In case you wonder, it's blocked on microsoft/TypeScript#16229.

Why is that a blocker?

@FrancescoBorzi

This comment has been minimized.

Copy link

FrancescoBorzi commented Jan 14, 2020

@kara are there any news about this?

@pullapprove pullapprove bot requested a review from AndrewKushnir Jan 29, 2020
@pullapprove pullapprove bot requested a review from IgorMinar Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

You can’t perform that action at this time.