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

remapFromFormGroup shouldn't allow extra props to be returned #162

Open
maxime1992 opened this issue Apr 6, 2020 · 2 comments
Open

remapFromFormGroup shouldn't allow extra props to be returned #162

maxime1992 opened this issue Apr 6, 2020 · 2 comments
Assignees
Labels
effort-1: minutes Will only take a few minutes to fix/create flag: breaking change This feature or fix will require a breaking change scope: lib Anything related to the library itself type: feature This is a new feature

Comments

@maxime1992
Copy link
Contributor

When creating a sub form we are strict and expect no missing properties, nor additional ones.
This lets us behind the scenes use setValue instead of patchValue which wouldn't be really safe as we could end up patching something completely unrelated to form without noticing.

When using the remap functionality, we say that:

  • Incoming value of type ControlInterface should be transformed into an "internal" value that is of type FormInterface
  • The output value must be of type ControlInterface as well, driven by the current form value (of type FormInterface)

The issue is the following: TypeScript uses structural typing instead of nominal typing which means that extra properties are not an issue. So when the form has some internal state, for example the current value of a select when working with polymorphic forms, it's totally fine to just return the form value. Even though it contains and additional key/value.

Coming from a form, this could easily lead to runtime issues as the server could perform checks and make sure that no extra properties are here, etc.

We might as well be strict and enforce this ourselves by doing a little trick.

@maxime1992 maxime1992 added scope: lib Anything related to the library itself effort-1: minutes Will only take a few minutes to fix/create type: feature This is a new feature labels Apr 6, 2020
@maxime1992 maxime1992 self-assigned this Apr 6, 2020
@maxime1992 maxime1992 added the flag: breaking change This feature or fix will require a breaking change label Apr 6, 2020
@maxime1992
Copy link
Contributor Author

I did give a go at this... And here's the best I could achieve:

type Impossible<K extends keyof any> = {
  [P in K]: never;
};

export type NoExtraProperties<T, U  = T> = U extends Array<infer V>
  ? NoExtraProperties<V>[]
  : U & Impossible<Exclude<keyof U, keyof T>>;

It gives us quite nice results as long as the types are nearly the same (U extends T....).
If that's not the case, it's all broken.

One thread/comment has a good summary about all that: microsoft/TypeScript#12936 (comment)

So we can assume this one is blocked by a 3rd party unless someone has a brilliant idea that I missed :)

@maxime1992
Copy link
Contributor Author

☝️ the above didn't work it always had edge cases not working, there's a typescript issue that we can track though which might make this doable at compile time: microsoft/TypeScript#12936

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort-1: minutes Will only take a few minutes to fix/create flag: breaking change This feature or fix will require a breaking change scope: lib Anything related to the library itself type: feature This is a new feature
Projects
None yet
Development

No branches or pull requests

1 participant