feature: support exact control types in FormGroup and FormArray

[sharshuv-quotient]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: CONTRIBUTING.md#commit
• 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?

[ ] Bugfix
[X] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

From the current documentation:

import { FormGroup } from '@ngneat/reactive-forms';

const group = new FormGroup<Profile>(...);
const address = group.getControl('name') as FormGroup<Profile['address']>;
const city = group.getControl('address', 'city') as FormControl<string>;

"Note that the return type should still be inferred." - This PR is fixing this issue.

What is the new behavior?

I'm enabling passing the type of the control / controls in addition to the current ability to pass the value type to FormGroup or FormArray.
For example, this way I can differ between those two, otherwise identical, Form Groups:

const group1 = new FormGroup<{
  name: string, // for primitive types, FormControl is inferred, instead of AbstractControl, which is what it was before.
  address: FormControl<{ city: string, street: string }>
}>({
  name: new FormControl('Shachar'),
  address: new FormControl({
    city: 'Tel Aviv',
    street: 'Ibn Gavirol'
  }),
})

const group2 = new FormGroup<{
  name: string, // for primitive types, FormControl is inferred, instead of AbstractControl, which is what it was before.
  address: FormGroup<{ city: string, street: string }>
}>({
  name: new FormControl('Shachar'),
  address: new FormGroup({
    city: new FormControl('Tel Aviv'),
    street: new FormControl('Ibn Gavirol')
  }),
})

Now I'll get automatic inference for the controls type:

const control1 = group1.getControl('address'); // FormControl<{city: string, street: string}> is inferred
const control2 = group1.getControl('address'); // FormGroup<{city: string, street: string}> is inferred

Does this PR introduce a breaking change?

[ ] Yes
[X] No

Note those changes are completely backward compatible! For evidence, I did not change existing tests and they all pass.

[NetanelBasal]

@itayod @danzrou please review.

[itayod]

will it still be typed for nested forms that includes FormGroup or FormArray without passing the FormGroup or FormArray types?
for example:

interface ProfileControls {
  firstName: string;
  lastName: string;
  address: {
    street: string,
    city: string
  } 
}

const profileForm = new FormGroup<ProfileControls>({
  firstName: new FormControl(''),
  lastName: new FormControl(''),
  address : new FormGroup({
    street: new FormControl(''),
    city  : new FormControl('')
  })
});

will city and still be typed?

[sharshuv-quotient]

Yes, in that case the form controls will be interpreted as:

{
  firstName: FormControl<string>;
  lastName: FormControl<string>;
  address: AbstractControl<{
    street: string;
    city: string;
  }>
}

Which means that:

profileForm.getRawValue().address.city; // string
profileForm.getControl('address'); // AbstractControl<{street: string; city: string}>
profileForm.getControl('address', 'street'); // AbstractControl<string>

Exactly as it was before these changes.

[itayod]

what about boolean?

[sharshuv-quotient]

You're right. Should probably add null and undefined too. (for types like number | null, for example)
Fixed.

[itayod]

could you add some more tests and examples for it?
for example, add a test for nested form and use getControl

[sharshuv-quotient]

Add a small test to this, and added this to the documentation.

[itayod]

does KeyValueControls still in use here? or we can delete it?

[sharshuv-quotient]

You're right, it's not being used.
Removed.

[itayod]

@sharshuv-quotient thanks for the quick reaction.

There is one more thing I would like to ask you to do, could you please run your branch on some big project to make sure there are really not breaking changes?

[sharshuv-quotient]

Unfortunately I don't have a big project already using this library.
I've been implementing this because I wanted those changes before I'm starting to use this library in my project.
I believe the tests are good enough, but if you have such a project you can try it your self or if you know of an OpenSource one tell me and I'll try.

[itayod]

@sharshuv-quotient sure we will also validate it from our side, though it would be nice if you could also try to test your branch inside your project (you could use npm link for that...).

[danzrou]

Can we also have this support in form builder?

[sharshuv-quotient]

Yes we can, but I think that's outside of the scope of this PR, since this can be merged without adding this to the form builder too.

[sharshuv-quotient]

• Resolved conflicts
• Successfully used this branch in a large project. I should say, however, that it's the first time we I'm this module at all (i.e. I replaced Angular's native module for the first time).

@itayod Are you currently checking this on your project?

[itayod]

@sharshuv-quotient could you resolve the failing test?

[sharshuv-quotient]

@itayod Fixed.

[NetanelBasal]

I'm not sure about this solution. You eliminate the need for casting, but you're also forcing me to override my model interface. In most cases, I'll have my model defined, for example - Article, and now I need to "duplicate" it just to get the auto casting.

[sharshuv-quotient]

I'm not forcing you to use a different interface. As I said, this is completely backward compatible.
But if you want your FormGroup to KNOW what kind of controls it has, you have to give it more information. That means a different, more details interface.

I don't see it as a duplicated interface, but as a different interface giving different information. If you want the controls interface to be connected to the value interface in some way (and avoid duplication as much as possible), you can do something like that:

interface Profile {
  firstName: string;
  lastName: string;
  skills: string[];
  address: {
    street: string;
    city: string;
  };
}

interface ProfileControls extends ControlsOf<Profile> {
  skills: FormArray<string>; // overriding AbstractControl<string[]>
  address: FormGroup<Profile['address']> // overidding AbstractControl<Profile['address']>
}

const fg = new FormGroup<ProfileControls>(...)

[Coly010]

Adding my two cents. I think the ideal goal is for the type system to understand that if you pass interface T, then it can map each key to a form control, form array or form group based on it's type signature.

export interface MyInterface {
    key1: string; // Becomes FormControl<string>
   groupKey: { nestedKey: string } // Becomes FormGroup<{ nestedKey: FormControl<string> }>
   arrayKey: string[]; // Becomes FormArray<string>
}

We don't exactly want the user to have to create a new interface to define this, we should be able to build this interface for them using the type system.

Otherwise we could simply tell users to cast their form value:

const formValue = this.form.value as T

[NetanelBasal]

@Coly010 exactly. But there is one exception here. You can use an object or an array with a FormControl.

[NetanelBasal]

A reasonable solution to solve this issue without introducing breaking changes is to create something like:

const fg = new FormGroup<ControlsOf<Profile>>(...)

[Coly010]

@Coly010 exactly. But there is one exception here. You can use an object or an array with a FormControl.

In those instances, it may be worth telling the user to provide an override for the keys that don't map based on type signature?
Or expose decorators that can append metadata to the properties in the interface. The downside of that is the coupling it creates.

[NetanelBasal]

Yes, but from my experience, it's not too common to pass an object/array to a FormControl, so I'll be OK with this.

[NetanelBasal]

@danzrou any thoughts?

[Coly010]

The only use-case i can think of is a multi-select form control. That returns an array of values for one FormControl right?

[NetanelBasal]

Usually, it will be a custom control accessor that takes an object/array. But again, not so common.

[sharshuv-quotient]

@Coly010 Correct me if I'm wrong, but what you are suggesting here is make the default interpretation of the value type regard object as a FormGroup and array as FormArray?
In my opinion this is a bad idea for two reasons:

1. It's a breaking change. Existing code with the FormGroup<Person> type which initialized "address" with a FormControl will now break. I think it's better to have the default interpretation ambiguous, than to assume potentially incorrect intentions of the consumer.
2. From my experience it's not too uncommon to use a FormControl with an object type / array type. I have a lot of use-cases in which I'm creating a custom form component (control value accessor) which value is an object / array. I do not think those usecases are as uncommon as you make them seem.

A reasonable solution to solve this issue without introducing breaking changes is to create something like:

const fg = new FormGroup<ControlsOf<Profile>>(...)

@NetanelBasal You mean here the ControlsOf interface is what @Coly010 suggested? I.e. - it's simply a utility interface? If so - we can totally add this, without really changing my current implementation. It's
a bit similar to the FlatControls interface I created to make stuff easier.

[NetanelBasal]

I mean that ControlsOf<Profile> should automatically do what you're doing manually:

interface ProfileControls extends ControlsOf<Profile> {
  skills: FormArray<string>; // overriding AbstractControl<string[]>
  address: FormGroup<Profile['address']> // overidding AbstractControl<Profile['address']>
}

[sharshuv-quotient]

OK, I'll add it, but I think it should be named DeepControls to differ it from the utility interface I have used inside the FormGroup implementation. (which regards objects / arrays as AbstractControls)

[danzrou]

I'll add my two cents here.

While developing this lib, we did try handling this by providing a type which will checkup the return values (@NetanelBasal ControlType).
However that resulted in tons of errors which some of them, as mentioned here, were due to being able to set objects on "single" controls and array as well (and it became even uglier when having deep nested objects).

That said, our main goal was to be able to provide this lib seamlessly with almost no effort so users could hook up and integrate it quickly and with minimum of writing.

As most of the solutions provided here are decent, they seem (at least from a first sight) to have a potential of not keeping things plain and simple as intended and adding (maybe indeed minimal) boilerplate for the user.

However, unfortunately, after doing a lot of thinking and also messing around a bit with strict types (#26) I couldn't yet think of a better option than providing the controls type explicitly by the user.

That's why I would go with the most simple solution advised here - a utility type that will resolve the values to their "natural" type:

1. Object -> FormGroup
2. Array -> FormArray
3. Primitive/other -> FormControl

[sharshuv-quotient]

Hey, I've added the discussed interface, which can be used optionally.
I have named it differently though: DeepControls, to avoid confusion with ControlsOf which is used in the FormGroup implementation (i.e. , explicitly, when you write FormGroup<Profile>. This is essential for backward compatibility). I've renamed ControlsOf to AbstractControlOf to make the difference more clear.

I did not implement the Overrides feature since I believe this should be a separate feature in a separate PR.

[sharshuv-quotient]

Resolved conflicts.
Can we merge this?

[itayod]

@sharshuv-quotient as we wrote above we wanted this pr to include only the controlsOf utility, could you please rename the utility you have created to controlsOf and clean the unrelated code?
Also please add the override functionality so it will be easy to use.

[sharshuv-quotient]

Done.

[itayod]

Hi @sharshuv-quotient the controlsOf looks good, but as we asked before could you please remove all of the other utils? as we don't want them to be a part of this PR...
please clean them from your commit and remove them from the docs...

In other words, try focus only on the ControlsOf feature and please don't touch anything else :)

[sharshuv-quotient]

The other utility you are referring to is FlatControls, right? Because AbstractControls is required for the FormGroup implementation. I can only make it non-exported if you want but it's essential for everything else to work (both for backward compatibility and both for using the new ControlsOf util)

[itayod]

The other utility you are referring to is FlatControls, right? Because AbstractControls is required for the FormGroup implementation. I can only make it non-exported if you want but it's essential for everything else to work (both for backward compatibility and both for using the new ControlsOf util)

correct, just remove the FlatControls for now and that's it :) thanks!

[sharshuv-quotient]

Resolved conflicts and remove FlatControls interface as discussed.
Will open another one for this after this is merged.

[sharshuv-quotient]

Resolved failing tests

[sharshuv-quotient]

Hey, can we merge this?

[itayod]

Hi @sharshuv-quotient sorry for the late response... could you resolve those conflicts and revert the "doc" changes that are not related to this feature?

after you do so I think we can merge it :D

[sharshuv-quotient]

Resolved conflicts. The doc seemed to be up to date though. Can you please point out what exactly is wrong?

[danzrou]

LGTM

[itayod]

@sharshuv-quotient well done!

[itayod]

@ShacharHarshuv I have noticed there is an issue that seems to be due to this pr.

interface User {
  id: number;
}
let user: User;
const form = new FormGroup({
  id: new FormControl(user.id)
});
// The type of control should be FormControl<number, any> instead it is AbstractControl<number | FormControl<number, any>>.
const control = form.getControl('id'); // AbstractControl<number | FormControl<number, any>>