Stricter types

[maxime1992]

Context

This is taking over #118 as this issue much bigger than just for adding new values into FormArrays.

Having a look into #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 🙌)

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:

• We always want that to be true or sometimes be able to skip the creation and enforce to pass only values of the type itself (without nullable props)
• checkFormResourceBeforeSending should be mandatory (could make things a bit more verbose...) or maybe just optional and would require to implement an interface. This may help at first and at some point we could potentially make it required? Also not sure how it'd work for people who are not using strict mode in TS

[zakhenry]

@maxime1992 how is the hook different to regular validation?

[maxime1992]

@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]

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]

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).

[zakhenry]

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 ***@***.***> 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 <#147?email_source=notifications&email_token=AAFQE2KVUCA5B3VC5AK3NMLRE26GZA5CNFSM4K3PHJ3KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENBQ6CA#issuecomment-591597320>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFQE2OLDDRUDM73GSM45RTRE26GZANCNFSM4K3PHJ3A> .

-- Zak Henry

[maxime1992]

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]

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]

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:

• 1. Not related to this issue but extending should createFormArrayControl accept null values for the value object #118: The definition should rather be:

protected transformToFormGroup(obj: ThreadMillStepOperation | null): NullableProps<ThreadMillStepOperation> | null {

• 2. The form can have null values even if the interface doesn't allow them. The way they're represented into the form, should always be NullableProps<MyInterface> as this is what's happening
• 3. Based on the previous point, I'd like to remind why I raised this issue in the first place: Validators won't help here for values in the interface that could be null too. So we should have some sort of validation mechanism to ensure that the form representation is actually matching the kind of output expected by the parent before sending it

[zakhenry]

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]

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 📝

• the form expects to receive a given type of object (input) but all the props are nullable as initially, if we try to create one we have nothing yet (or it may be partial)
• the form guarantees that it will not send back the updated data to the parent while:
  • the form is invalid (any kind of validator can be set)
  • the form values does not match the initial type passed (this time without the nullable as it's the output)

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.