search

cloudnc / ngx-sub-form

Utility library for breaking down an Angular form into multiple components
https://cloudnc.github.io/ngx-sub-form
MIT License
315 stars 33 forks source link

Stricter types #147

Open maxime1992 opened 4 years ago

maxime1992 commented 4 years ago

Context

This is taking over https://github.com/cloudnc/ngx-sub-form/issues/118 as this issue much bigger than just for adding new values into FormArrays.

Having a look into https://github.com/cloudnc/ngx-sub-form/pull/146 and the stricter settings, I do realize that here:

  public listing$: Observable<NullableObject<OneListing>> = this.route.paramMap.pipe(
    map(params => params.get('listingId')),
    switchMap(listingId => {
      if (listingId === 'new' || !listingId) {
        return of(null);
      }
      return this.listingService.getOneListing(listingId);
    }),
    map(listing => (listing ? listing : this.emptyListing())),
  );

We're defining the listing$ as Observable<NullableObject<OneListing>>.

and here:

<app-listing-form
  [disabled]="readonlyFormControl.value"
  [listing]="listing$ | async"
  <----------------------------------------------
  (listingUpdated)="upsertListing($event)"
></app-listing-form>

We're passing that into the root form. But the root form takes as a parameter a OneListing, not a NullableObject<OneListing>:

https://github.com/cloudnc/ngx-sub-form/blob/v5.0.1/src/app/main/listing/listing-form/listing-form.component.ts#L39

@Component({
  selector: 'app-listing-form',
  templateUrl: './listing-form.component.html',
  styleUrls: ['./listing-form.component.scss'],
})
export class ListingFormComponent extends NgxRootFormComponent<OneListing, OneListingForm>

(which now throw a TS error with strict mode ON :raised_hands:)

Issue

In a world where we'd just make edits, we could skip the NullableObject because we'd only receive objects of the exact type. But in reality we also want to have the possibility to create new objects (therefore they'd have all or some properties set as null when they're passed as input).

A good example of that is the one above with listing$: Observable<NullableObject<OneListing>>. We generate a new ID and pass all the other props as null.

Other example, if the form is being reset (without the default values). They'll all be set to null.

Question

Should we always provide an API that would make the input required + nullable props + nil and offer a new hook to run a strict check to make sure all the non nillables values are defined?

Example of a new API

We could have a new type:

export type DataInput<ControlInterface> =
  | NullableObject<Required<ControlInterface>>
  | null
  | undefined;

And for a component instead of having:

  @DataInput()
  @Input('listing')
  public dataInput: Required<OneListing> | null | undefined;

It'd be

  @DataInput()
  @Input('listing')
  public dataInput: DataInput<OneListing>;

Besides the friendlier syntax, DataInput uses NullableObject which is what I want to focus on here. This means that we'd be able to pass null properties on the object (but they should still all be defined), and as in the Output we still want a OneListing we could have a hook that'd check for the null values and throw an error if needed. This hook would be useful to fill up the gap between what we want and the checks ran on the FormGroup (in case we forget to add a Validators.required for example).

Demo:

export class ListingFormComponent extends NgxRootFormComponent<
  OneListing,
  OneListingForm
> {
  @DataInput()
  @Input('listing')
  public dataInput: DataInput<OneListing>;

  @Output('listingUpdated')
  public dataOutput: EventEmitter<OneListing> = new EventEmitter();

  // classic methods here...

  protected checkFormResourceBeforeSending(
    resource: NullableObject<OneListing>
  ): resource is OneListing {
    // do the check
  }
}

if checkFormResourceBeforeSending would return false then we should internally throw an error (as it should never happen).

Random thoughts

I wonder if:

zakhenry commented 4 years ago

@maxime1992 how is the hook different to regular validation?

maxime1992 commented 4 years ago

@zakhenry regular validation doesn't apply any kind of type checking. Therefore, if you forget to apply a required somewhere chances are it'll trigger bugs upstream (trying to access a property of a null value, etc).

Whereas the hook there would bring some nice type safety. BUT writing this I realize that the hook definition above wouldn't really help to make sure all the properties are matching.

It should rather be:

  protected checkFormResourceBeforeSending(
    resource: NullableObject<OneListing>
  ): undefined | OneListing {
    // do the check
  }

At the end of the function it'd return the resource and before it'd have to make a bunch of checks to make sure that all the required props have been entered otherwise return; and if the function returns undefine ngx-sub-form would throw an error.

zakhenry commented 4 years ago

Eh I really don't think we should be going to this level of enforcement of type safety - form interface validity should be controlled with validation, data output validation should be enforced by the type system. Given the developer has to write a mapping function anyway if it is a remap component, they have to get the type right. If it is not remapped, then the validation should manage it.

maxime1992 commented 4 years ago

Eh I really don't think we should be going to this level of enforcement of type safety

I'm not convinced, I think we should. Maybe there's another way of applying that but as of today I don't think we're strict enough and it can easily lead to some bugs.

form interface validity should be controlled with validation

Well, it's a bit like saying let's code in JS and have a lot of tests instead of using Typescript + doing some tests :stuck_out_tongue: (yes, I do exaggerate here :angel:).

Within our internal app we do have a lot of forms and most of them are built with ngx-sub-form. But if the API changes at some point and some properties previously not required would become required for example, while the build would still be fine, we'd end up with runtime errors if we're not super careful about the changes (which is not ideal, it should be a no brainer for that case at least).

I'm trying here to find a solution to close the gap between validation and type safety.

data output validation should be enforced by the type system

Totally agree. But it's not the case today. You can totally end up returning an object with all his props set to null today and it'd be fine (for TS), while it shouldn't. Because you could reset a value, remove a child sub form which would set its own value in the parent to null, etc. We need runtime checks to guarantee this safety IMO.

Given the developer has to write a mapping function anyway if it is a remap component, they have to get the type right

The remap is not here for that I think and I'd like to be able to have that kind of type safety without implementing remap (it would mix multiple concepts and makes things harder IMO).

zakhenry commented 4 years ago

What Iā€™m getting at is we should come up with a solution that is not requiring NullableProps. I think that should be considered an antipattern and we may need to find better ways around that for dealing with oneOf types. It may be the case that it is incorrect to have child components create values when presented with null values?

On Wed, 26 Feb 2020 at 7:20 pm, Maxime notifications@github.com wrote:

Eh I really don't think we should be going to this level of enforcement of type safety

I'm not convinced, I think we should. Maybe there's another way of applying that but as of today I don't think we're strict enough and it can easily lead to some bugs.

form interface validity should be controlled with validation

Well, it's a bit like saying let's code in JS and have a lot of tests instead of using Typescript + doing some tests šŸ˜› (yes, I do exaggerate here šŸ‘¼).

Within our internal app we do have a lot of forms and most of them are built with ngx-sub-form. But if the API changes at some point and some properties previously not required would become required for example, while the build would still be fine, we'd end up with runtime errors if we're not super careful about the changes (which is not ideal, it should be a no brainer for that case at least).

I'm trying here to find a solution to close the gap between validation and type safety.

data output validation should be enforced by the type system

Totally agree. But it's not the case today. You can totally end up returning an object with all his props set to null today and it'd be fine (for TS), while it shouldn't. Because you could reset a value, remove a child sub form which would set its own value in the parent to null, etc. We need runtime checks to guarantee this safety IMO.

Given the developer has to write a mapping function anyway if it is a remap component, they have to get the type right

The remap is not here for that I think and I'd like to be able to have that kind of type safety without implementing remap (it would mix multiple concepts and makes things harder IMO).

ā€” You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/cloudnc/ngx-sub-form/issues/147?email_source=notifications&email_token=AAFQE2KVUCA5B3VC5AK3NMLRE26GZA5CNFSM4K3PHJ3KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENBQ6CA#issuecomment-591597320, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFQE2OLDDRUDM73GSM45RTRE26GZANCNFSM4K3PHJ3A .

-- Zak Henry

maxime1992 commented 4 years ago

we should come up with a solution that is not requiring NullableProps

Why? I do feel like allowing internally the object to be of type NullableProps<Resource> makes sense. In a form you don't always have values set to this would be a more accurate representation of the object we can have internally.

think that should be considered an antipattern and we may need to find better ways around that for dealing with oneOf types

Just to be clear, I'm not saying this issue is related to oneOf types. Even if you don't have a oneOf it should still be NullableProps<Resource> IMO.

It may be the case that it is incorrect to have child components create values when presented with null values?

I don't understand what you mean here?

zakhenry commented 4 years ago

That's where the disagreement is then I think. My opinion is that a form control should declare the type that it is expecting in the input and the type that it will emit in the output. For simplicity they should be the same type. If there is some need for form reasons (therefore within the sub-components owned concerns) for that type to be NullableProps, then it should be doing a remap to handle that type mismatch. The common case ought to be that remap is not needed, and the full interface is what is passed by the parent.

Within this set of principles, I'm sure we will be able to come up with a simpler (& safer) solution than what we have now, rather than a more complex one

maxime1992 commented 4 years ago

Not following up on last comment just sharing another use case for stricter types.

The current definition of the transformToFormGroup is the following: 

protected transformToFormGroup(obj: ControlInterface | null, defaultValues: Partial<FormInterface> | null): FormInterface | null;

Note that obj can be either ControlInterface or null. And to make our life easier, ngx-sub-form accepts as a return type FormInterface | null and if it's null, it'll set all the form values to null. This way we don't have to make a check:

if (!obj) {
  return {
    prop1: null,
    prop2: null,
    // ....
    prop49: null,
    prop50: null,
  }
}

We can simply do instead:

if (!obj) {
  return null;
}

Either way, all the values of the form can end up null. So my point is:

zakhenry commented 4 years ago

I still think we should drop the use of NullableProps - it might be convenient, but it is a violation of the interface. Instead it would be better in that common case to do

if (!object) {
  return this.getDefaultValues();
}

that way the interface can be very strongly trusted to be the correct value. If it is valid for some reason to have nullable props, then that should be declared on the form type

maxime1992 commented 4 years ago

As we may not always have default values to provide, getDefaultValues definition is:

protected getDefaultValues(): Partial<FormInterface> | null;

So it's pretty much the same except that instead of having null values, we'll have some of them undefined. Once again, the internal representation of the data will be incorrect for some values that are not nullable.

Let's take a simple example: A user.

What would you use as default values for name and age? No way to know.

You want to set those values to null initially then guarantee that before sending the new object to the parent, the "outside" interface is fulfilled.

I think we can think of forms as a contract :memo:

I feel like enforcing that kind of behavior makes it clear for everyone (parents and also internally for the form). With strict types turned on, it'd also enforce better safety in TS or HTML when working with the formGroupValues. Imagine you've got an object of type:

interface Animal {
  id: string;
  name: string;
}

interface Person {
  id: string;
  name: string;
  age: number;
  favouriteAnimal: Animal;
}

If you're working on a person form and accessing formGroupValues, currently the type will be Person while IMO it should be NullableProps<Person>.

If you try to do:

console.log(formGroupValues.favouriteAnimal.name)

You could get an error if the favouriteAnimal isn't set yet. And even though it's not optional on the Person interface, it's ok to have it as potentially null internally into the form. The only thing we should provide is a way to ensure that before sending that object back to the parent, it fulfill the interface and has a favouriteAnimal.

zakhenry commented 4 years ago

The only thing we should provide is a way to ensure that before sending that object back to the parent, it fulfill the interface and has a favouriteAnimal.

but not at runtime. The typesystem should be used to ensure correctness of contract, not the runtime.

With your example, if the requirement is that an animal must have the fields set, then the developer should do

class AnimalForm extends NgxSubForm<Animal, NullableProps<Animal>> {}

The default should probably just become FormInterface | null?

I realise I'm describing a pretty big breaking change, but I really don't want to have dangerous (thrown error) bugs only appear at runtime.

maxime1992 commented 4 years ago

but not at runtime

I disagree here as the form could be patched from anywhere really. And it should be perfectly fine patching the form with null values to reset it while you expect the value to match the interface before it gets send to the parent.

The default should probably just become FormInterface | null?

Nop all the keys can be nillable really. And the form will always be defined no matter what so I dont think | null is needed here.

I really don't want to have dangerous (thrown error) bugs only appear at runtime.

You'd just ensure that an object is matching a type. It'd be pretty much equivalent to a type guard.

zakhenry commented 4 years ago

all the keys can be nillable really

can we focus on this? I'm saying that it should be possible to completely eliminate this scenario.

I think that is where all of the complexity is creeping in, and if it can be eliminated we will save so much complexity all over the place.

it should be perfectly fine patching the form with null values to reset it

why? maybe it is acceptable for a parent form to pass null to a child form (but even in this case the child should not be creating values), but never a partial object.

maxime1992 commented 4 years ago

Discussed IRL. Not sure what to do about it but here are some notes:

The issues we have currently

Different implementations for different use cases

Idea 1: Only make sure that a form has its required values set

This would be the "light" one. It would only make sure at a compiler + runtime level that the values in the form are fulfilling the interface in a (TS) "strict" way. By TS strict way I'm referring to make sure that a type T for a value doesn't end up being Nillable<T>.

The changes required for that would be easy to implement. Devs would have to define something in their forms like the following:

export interface User {
  id: string;
  name: string;
  age: number;
  carModel?:string;
}

export class MyForm extends NgxSubForm<User> {
  // ...

  protected requiredKeys: RequiredKeys<User> = ['id', 'name', 'age'];

  // ...
}

The RequiredKeys interface would be type safe and force devs to list the required properties. Behind the scenes, right before the value would be sent to the parent, it'd loop over the required properties and make sure that the corresponding form values are not null or undefined.

Idea 2: Make sure that everything in the form matches the interface

This idea would be much more "intrusive" and require a more changes from the devs. But it'd also fill a much bigger gap: The gap between TS and HTML that is not type safe. In the example I talked about at the beginning of this post (input of type text which should be of type number) would be caught here.

Here's how it could look like:

export type ValueType<FormInterface> = Record<keyof FormInterface, Unknown>;

export class MyForm extends NgxSubForm<User> {
  // ...

  protected validateOutput(value: ValueType<User>) value is User {
    // here check everything. 
    // that required values are defined, that strings are string, numbers are numbers, etc
  }
}

This would help us ensure that whether we're updating a resource (easy case) or creating a resource (which can have null values), the output will always be of the expected type (in that case, User).

@zakhenry did I forget anything?