feat(ControlsType): added typings for AbstractControls + FormArrays

[ntziolis]

Allow for partially typed form controls when accessing controls via formGroupControls. Typed methods are:

• setValue
• patchValue
• valueChanges
• value

There is a caveat with this approach:

• since the controls are now not of type AbstractControl anymore, instead it is TypedAbstractControl
• This means a direct cast to to FormControl or FormGroup is not possible. Instead one has to use as unknown before casing to FormControl or FormGroup

I feel this caveat is acceptable. Should you feel otherwise I suggest adding a third generic parameter to NgxSubForm that allows to specify which type should be used for formGroupControls so it would be up to the dev to choose to accept the above mention caveat.

[ntziolis]

Looking into why tests fail builds were passing locally

[ntziolis]

After going through the travis log it is not clear to me what the issue is. All tests / specs are marked as passed no failures reported.

[maxime1992]

@ntziolis thanks for this PR, I took a quick look and like where this is going! 👏

To explain a bit what's happening on CI, the only command failing is:

The command "yarn run prettier:check" exited with 1.

So nothing to really worry about. We use Prettier to automatically format our code and we've got a check on CI to make sure everything's fine.

The only command you need to run is: yarn run prettier:write then amend your commit and push force :)

I'll make a better review ASAP 👍

[maxime1992]

Really good idea 👍 thanks again for contributing btw!

While we're at it, I think it'd be nice to also update TypedFormGroup as currently I think it only handle type safety from formGroupControls but not the formGroup itself, does it?

formGroup.controls can probably be of type ControlsType<FormInterface> too. And setValue, patchValue, valueChanges can also be improved on it :)

[maxime1992]

It would also be nice to edit the readme where needed to reflect those changes

[ntziolis]

Not done yet, will ping when ready to review.

[ntziolis]

@maxime1992 I have just realized the following:

• We have the FormInterface type to build typing on
• But for properties that represent For objects / arrays we do not know if
  • they are handled by another sub form which means they should be typed as FormControl
  • or if they are handled on the current sub form which would mean they actually would have to be typed as FormGroup or FormArray respectively

This affects setValue / patchValue. The options I see are:

• no typing for setValue / patchValue
• type value part but do not type options (right now its AbstractControl with no typing for options either)
• or always assume FormControl and show not existing options for FormArrays

What do you think? Any other options I overlooked?

UPDATE:
This actually applies generally and not just to the new typings. Even prior to the typing update in this PR there is an assumption made that when there is an array in the FormInterface that the control the user created is a FormArray and that it is handled on the current sub form.

In our code we have extracted arrays into a dedicated sub form that only handles the array. This allowed us to create generic array value sub form base class that provides helper functions to handle array forms easily. This class again is based on a single value sub form

[ntziolis]

While we're at it, I think it'd be nice to also update TypedFormGroup as currently I think it only handle type safety from formGroupControls but not the formGroup itself, does it?

TypedFormGroupis now also using the new typings.

[ntziolis]

All done. Let me know your thoughts.

[ntziolis]

Anything I can do to help get this PR merged in?

[zakhenry]

@maxime1992 this all looks great to me, happy for merge. No diff in the example repo so I’m fairly confident this is non-breaking downstream

[ntziolis]

All done:

• Ivy compatible typings
• Resolved all comments

[maxime1992]

Caught 2 minor things, should be good to merge straight after that :)!

[ntziolis]

Some food for thought:

• The option parameter on AbstractControl.setValue has no properties
• This means we would not provide any typing (same as prior to this PR)
• However TypedAbstractControl is only exposed externally when the property is not an array (so its either a form control or a form group)
• Since the option parameter of FormGroup.setValue is a subset of FormControl.setValue option parameter I chose to provide maximum typing with the downside of overtyping for FormGroups

While writing this I had another idea:

• Instead of not providing any typing we could provide multiple typing options:
  options?: Parameters<FormControl['setValue']>[1] | Parameters<FormGroup['setValue']>[1]
• This would provide developers with a hint to be careful while still proving some typing instead of none

[maxime1992]

Oh you're right for

The option parameter on AbstractControl.setValue has no properties

Thanks for bringing that up 👍

• Instead of not providing any typing we could provide multiple typing options:
  options?: Parameters<FormControl['setValue']>[1] | Parameters<FormGroup['setValue']>[1]
• This would provide developers with a hint to be careful while still proving some typing instead of none

Yes, I feel like this would be the best approach here. @zakhenry any other suggestion?

[zakhenry]

Yep I see the problem here, however I would recommend that we make the signature

export type ControlsType<T> = {
  [K in keyof T]-?: T[K] extends any[] ? TypedFormArray<T[K]> : TypedFormControl<T[K]> | TypedFormGroup<T[K]>;
};

it is a more realistic outcome - you can't construct an AbstractControl directly so I think it is better to be explicit and say that this key is a control or a group, who knows, you'll have to use a type-guard to test it.

[ntziolis]

I really like this approach. @zakhenry picking up on an earlier comment I made:
Even when a property on the FormInterface is an array we do not know if this property is actually a FromArray (array handled on the current subform) or a FormControl (array handled by a downstream subform). Only for non array non object properties on the FormInterfacecan be certain it has to be a FormControl.

So we should really be typing this as follows:

export type ControlsType<T> = {
   [K in keyof T]-?: T[K] extends any[] 
      ? TypedFormControl<T[K]> | TypedFormArray<T[K]> 
      : T[K] extends object 
           ? TypedFormControl<T[K]> | TypedFormGroup<T[K]>
           : TypedFormControl<T[K]>;
};

The thing is:
The lib does make an assumption that it is a form array and provides the controls with signature of [] property to users today. That means changing the typing to be actually correct would likely break existing usage scenarios. In addition it might make the library less useful for array use cases when one still has to explicitly specify the typing to use when they wanne access the controls property as an array.

For now I have updated the PR with the typings provided in your last comment . But wanted to make sure the earlier comment I made is better understood.

[ntziolis]

FYI: The CI process seem to timeout sometimes and I have no way to re-trigger a build besides pushing a change. Same code with a space then runs through without a hitch.

[maxime1992]

@ntziolis I gave this a go on our app at work and I noticed that something is odd when dealing with arrays. I've made a minimal repro:

interface Animal {
  name: string;
}

interface AnimalsForm {
  animals: Animal[];
}

export class A extends NgxSubFormRemapComponent<Animal[], AnimalsForm> {
  constructor() {
    super();
    this.formGroupControls.animals.setValue();
  }
}

When using setValue it expects an array of array of the resource type

[ntziolis]

I have an idea what the issue might be

[maxime1992]

I have an idea what the issue might be

Oh cool. BTW you'll need to rebase as #145 got merged 👍

[maxime1992]

FYI: The CI process seem to timeout sometimes and I have no way to re-trigger a build besides pushing a change. Same code with a space then runs through without a hitch.

I just saw that on your PR. It appears to already have an issue reported on Cypress side: cypress-io/cypress#5965

I did re trigger your build 👍

[maxime1992]

@zakhenry I think this is now good to go 🔥

[maxime1992]

🎉 This PR is included in version 5.1.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[maxime1992]

@ntziolis I'd like to post a Tweet to announce the better typings on ngx-sub-form and I'd like to give you back the credit for it 👍 Do you have a Twitter account that I can use? 😸

[ntziolis]

wupwup. much appreciated and yes I do, it's the same as my github handle.

[maxime1992]

wupwup. much appreciated and yes I do, it's the same as my github handle.

I tried this morning and it didn't work 😂 not sure what I did wrong but it's working now, thanks 👍

Will do asap!

EDIT: done https://twitter.com/maxime1992/status/1232967540370018304