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

Runtype definitions are not working as expected #294

Open
elias-garcia opened this issue Dec 14, 2021 · 7 comments
Open

Runtype definitions are not working as expected #294

elias-garcia opened this issue Dec 14, 2021 · 7 comments

Comments

@elias-garcia
Copy link

elias-garcia commented Dec 14, 2021

Hi! I've found an issue when you try to create a Runtype from a TypeScript type with an optional property in it.

So, for example, imagine that I have this TS type:

type A = {
  foo: string;
  bar: string | undefined;
};

This type is coming from the domain of my application, so I want to have it as a TS and not having it depending on an external library, so I'm able to change the runtime decoding at any time without affecting the rest of the app. I mean, I don't want to infer my domain from an external library doing:

type A = Static<typeof A>;

So okay, no problem, I should be able to do:

const A: RunType<A> = Record({
  foo: String,
  bar: Optional(String)
})

And yeah, it works perfectly, no compilation errors as we expect. But what if I forgot the Optional like this?

const A: RunType<A> = Record({
  foo: String,
  bar: String
})

I don't get any compilation error, but the bar property is defined as string | undefined in my A type, so it should take into account that it could be undefined. What if I'm trying to decode a response from the API and it's returning undefined? The app is going to break in runtime, so then what's the purpose of using a library to prevent this kind of errors?

And I don't get any compilation error neither if I try to add more extra properties to the runtype definition like:

const A: RunType<A> = Record({
  foo: String,
  bar: String,
  extraPropNotInA: Number,
})

The same as before, the API is not going to return extraPropNotInA and when trying to decoding the response the app is going to break again! So, where is the strictness?

For example, in io-ts I get a compilation error in both cases, like it should be in my opinion. It really guarantees that I'm not going to face a runtime error. But trying another libraries like zod and runtypes I've seen that all of them fail in this kind of scenarios. Is this behavior intended? And if it's, why? Thanks!

@elias-garcia elias-garcia changed the title Unexpected behavior when working with undefined union definitions Runtype definitions are not working as expected Dec 15, 2021
@yuhr
Copy link
Collaborator

yuhr commented Dec 15, 2021

Thanks, this kind of use case is exactly what I've been thinking recently. First of all, your examples are a bit wrong as it's not wrapped within Record(). The correct form is like:

const A: Runtype<A> = Record({
  foo: String,
  bar: String,
})

The problem is, the constraint that requires typeof Record({ foo: String, bar: String }) must be assignable to Runtype<{ foo: string; bar: string | undefined; }> is actually valid, that is, Record({ foo: String, bar: String }).check() returns { foo: string; bar: string; } which is obviously assignable to { foo: string; bar: string | undefined; }. This is how TS's subtyping works. You can notice the opposite case does emit an error expectedly:

type A = {
  foo: string
  bar: string
}
const A: Runtype<A> = Record({
  foo: String,
  bar: String.Or(Undefined),
})
// Types of property 'bar' are incompatible.
//   Type 'string | undefined' is not assignable to type 'string'.
//     Type 'undefined' is not assignable to type 'string'.

I guess what you really want is a dedicated API that statically ensures given runtype to be not only assignable to but also exactly Runtype<A>. For example:

type Exactly<T, U> = [T, U] extends [U, T] ? true : false
const Ensure: <T>() => <U>(
  u: Exactly<Runtype<T>, Runtype<U>> extends true ? Runtype<U> : never,
) => Runtype<T> = () => u => u as any

type A = {
  foo: string
  bar: string | undefined
}
const A: Runtype<A> = Ensure<A>()(
  Record({
    foo: String,
    bar: String,
  }),
)
// Argument of type 'Record<{ foo: String; bar: String; }, false>' is not assignable to parameter of type 'never'.

This is just a rough idea, we'd like to get more understandable error messages in real-world usage, but it works. By the way, could you elaborate on how you can write such constraint in io-ts? Maybe we can consider it as a reference in order to design our API.

@amonsosanz
Copy link

could you elaborate on how you can write such constraint in io-ts?

Hi, I reported a similar case to Zod's repo and wrote an example on how to write such constraint in io-ts.

@yuhr
Copy link
Collaborator

yuhr commented Dec 15, 2021

In io-ts it seems that TS emits an error on the signature of the encode method. I think this is caused by incompatibility of the parameter types, since parameter types effectively “invert” normal type assignability checks, for example:

type A = {
  foo: string
  bar: string | undefined
}

type B = {
  foo: string
  bar: string
}

type F<A> = (a: A) => void

const f: F<A> = (b: B): void => {}
// Types of property 'bar' are incompatible.
//   Type 'string | undefined' is not assignable to type 'string'.
//     Type 'undefined' is not assignable to type 'string'.(2322)

TS Playground

So, the type safety of runtype definitions is probably achieved by having a phantom function signature in the Runtype interface. For example:

export interface RuntypeBase<A = any> {
  _phantom: (a: A) => never;
  ...
};

Just confirmed it works. The error message with the OP's example is like:

Types of property '_phantom' are incompatible.
    Type '(a: { foo: string; bar: string; }) => never' is not assignable to type '(a: A) => never'.
      Types of parameters 'a' and 'a' are incompatible.
        Type 'A' is not assignable to type '{ foo: string; bar: string; }'.
          Types of property 'bar' are incompatible.
            Type 'string | undefined' is not assignable to type 'string'.
              Type 'undefined' is not assignable to type 'string'.

But I feel it quite hacky 🤣

@amonsosanz
Copy link

So if I understood correctly, this works "by chance" on io-ts because it has the ability to encode/serialize back the decoded entities, and it's in that encode function where TS complains about the inconsistency.

Assuming the hacky _phantom is not an option, are you considering other ways for runtypes to natively provide this kind of check at compile time?

@elias-garcia
Copy link
Author

BTW, thanks for providing all that information regarding TypeScript types @yuhr like this:

since parameter types effectively “invert” normal type assignability checks

I didn't know that! I agree that _phantom is not an option as it's a feature in io-ts and here it should be a weird hack 😂 I'm trying to think a little bit in the Exact and Ensure helpers that you provide. I think that they could be a good option to have really strict code. And well, probably the only option without doing weird things!

@yuhr
Copy link
Collaborator

yuhr commented Dec 16, 2021

So if I understood correctly, this works "by chance" on io-ts

Yeah, I believe it wouldn't be something intended.

Assuming the hacky _phantom is not an option, are you considering other ways for runtypes to natively provide this kind of check at compile time?

Definitely, we must cover this. I said hacky, but didn't mean it's an enough reason to stop taking that option (I see the current codebase of runtypes leverages a lot of dirty hacks already).

I just wonder which way provides better experience to users. _phantom solution is, from the users' point of view, just a simple type annotation like const A: Runtype<A> = ..., which is quite idiomatic way to express a constraint of a value, while it might look confusing about the error message, as it reports the weird property _phantom which is actually not used anywhere. If such confusion can be solved in some way or considered trivial, I'd say this is the better choice. Also maybe we should take #220 into account.

On the other hand, Ensure API looks somewhat more focused on it's purpose. It's a non-invasive solution, so probably it won't be impact on the type instanciation depth problem #212. However, so far I have no clue how to get more understandable error messages (like _phantom solution provides).

I'm going to tinker around how we can make things more sophisticated.

@elias-garcia
Copy link
Author

elias-garcia commented Dec 17, 2021

On the other hand, Ensure API looks somewhat more focused on it's purpose. It's a non-invasive solution, so probably it won't be impact on the type instanciation depth problem #212. However, so far I have no clue how to get more understandable error messages (like _phantom solution provides).

I would say that this Ensure function is a good approach but I don't like the fact that it's an IIFE. I know that the IIFE is required to get both generics T and U but it looks quite weird when you use it... I also agree that the error message does not provide any kind of information and it's quite difficult to understand what's going on.

I just wonder which way provides better experience to users. _phantom solution is, from the users' point of view, just a simple type annotation like const A: Runtype = ..., which is quite idiomatic way to express a constraint of a value, while it might look confusing about the error message, as it reports the weird property _phantom which is actually not used anywhere.

I also agree on that this is the solution which provides better experience to the users. And what I like the most of this approach is that the strict type checking is not optional if you define a Runtype. I think that if we can find a good name for this _phantom function like _checkIdentity or _checkTypeIdentity it could be the best option for sure :)

Apart from that, me and @amonsosanz ended up using this temporal function to have strict type checking. I'm posting our final solution just in case it helps someone that is finding this stricness:

export const StrictSchema: <T>() => <U>(
  u: Exact<Runtype<T>, Runtype<U>> extends true
    ? Exact<Required<T>, Required<U>> extends true
      ? Runtype<U>
      : never
    : never
) => Runtype<T> =
  () =>
  <T>(u: unknown) =>
    u as Runtype<T>;

Of course, as you've said before, the drawback is the error message when the Runtype doesn't match your interface:

Argument of type '...' is not assignable to parameter of type 'never'

Thank you a lot for your help, all the credits to you @yuhr. We just removed the as any at the end to avoid linting errors with our config and added an extra check to be sure that all optional properties are also checked :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants