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

3.20: Input type of a schema with a coercion #1760

Closed
RobinTail opened this issue Dec 26, 2022 · 14 comments
Closed

3.20: Input type of a schema with a coercion #1760

RobinTail opened this issue Dec 26, 2022 · 14 comments
Labels
bug-confirmed Bug report that is confirmed stale No activity in last 60 days

Comments

@RobinTail
Copy link
Contributor

RobinTail commented Dec 26, 2022

I believe there is a confusion with the way how the new coerce feature works together with the inferring of the input type.

Here is the example with the ZodString having coerce enabled:

const schema = z.coerce.string(); // ZodString
console.log(schema.parse(123)); // "123", no errors
type SchemaInput = z.input<typeof schema>; // string
const input: SchemaInput = 123; // TS2322: Type 'number' is not assignable to type 'string'.

Under the hood, as per my understanding, the schema actually accepts any type of the input, making a String out of them.
However, inferring the input type of such schema returns string, so the coerce feature is not taken in account on the typescript level.

I believe, the z.coerce.string() should create ZodType<string, ZodStringDef, any> (input type — any, not equal to the output type — string).

what do you think, @colinhacks ?

@JacobWeisenburger
Copy link
Contributor

JacobWeisenburger commented Dec 26, 2022

I have confirmed this bug. Not exactly sure how to fix it though. Any ideas?

@RobinTail
Copy link
Contributor Author

RobinTail commented Dec 29, 2022

Not exactly sure how to fix it though. Any ideas?

Well... in order to preserve the backward compatibility, the following idea comes to my mind.
I'm going to explain it on the example of ZodNumber, however it could be applied to any class currently having coerce feature.

Consider the current declaration:

class ZodNumber extends ZodType<number, ZodNumberDef> { }
// input type is equal to the output one by default (number)

where coerce is the property within ZodNumberDef:

interface ZodNumberDef extends ZodTypeDef {
  checks: ZodNumberCheck[];
  typeName: ZodFirstPartyTypeKind.ZodNumber;
  coerce: boolean;
}

We need to make coerce to be a type argument with a backward compatible default. Here is the possible solution:

interface ZodNumberDef<Coerce extends boolean = false> extends ZodTypeDef {
  checks: ZodNumberCheck[];
  typeName: ZodFirstPartyTypeKind.ZodNumber;
  coerce: Coerce;
}

class ZodNumber<Coerce extends boolean = false>
  extends ZodType<
    number,     // output type
    ZodNumberDef<Coerce>,
    Coerce extends true ? any : number     // conditional input type
  > { }

It's only the beginning, for demonstrating a possible way of fixing this.
Additional corresponding changes would be required to align other statements that create the class instances.

@JacobWeisenburger

@RobinTail
Copy link
Contributor Author

@JacobWeisenburger , I made a fix #1793 — please review.

aleclarson added a commit to aleclarson/zod that referenced this issue Jan 16, 2023
@aleclarson
Copy link

aleclarson commented Jan 16, 2023

I rewrote @RobinTail's fix to not be a breaking change by introducing new coerced types (eg: ZodCoercedString) and updating the z.coerce type signatures accordingly.

aleclarson@35e228f

The z.input results are aligned with each type's coercion expression. For example, doing z.input<ZodCoercedBigInt> will resolve to string | number | bigint | boolean, since that's what the BigInt constructor accepts (according to its TypeScript definition).

@colinhacks @RobinTail @JacobWeisenburger Does this sound like an acceptable solution?

edit: Well, I guess one could argue it's still a breaking change, since z.input results might be relied upon in application code. 😔 Oh, btw, I haven't done the Deno changes yet.

@maxArturo
Copy link
Contributor

@aleclarson I took a quick look and having types with the actual expected coercion inputs seems reasonable to me, FWIW. It forces users to be explicit about disregarding the expected input types for those that need it (bigint and date maybe).

I'm not sure what would happen in re: to the coercion types sometimes not being assignable back to the zod "main" types, since I haven't worked in userland code that does that extensively. That would be a concern I think.

@RobinTail
Copy link
Contributor Author

I rewrote @RobinTail's fix
aleclarson@35e228f

@colinhacks @RobinTail @JacobWeisenburger Does this sound like an acceptable solution?

Didn't you forget to cover the case like z.number({coerce: true})

@aleclarson

@aleclarson
Copy link

@RobinTail ah yes, I did 😞

@RobinTail
Copy link
Contributor Author

That's why I went with the approach in my PR. I believe it's backward compatible while having defaults where type params are introduced.
However, if you have a better idea on how to achieve fixing this issue, I'd be happy if it would be fixed by any means, @aleclarson

@colinhacks
Copy link
Owner

This was a tradeoff with adding the coercion logic directly into the respective primitive schema types (ZodString, etc.). I could have introduced a ZodCoerce type that could properly represent the Input type, but then the various ZodString methods wouldn't have been available on z.coerce.string().

I don't like the idea of adding any generics to ZodString I'm afraid - it clutters up the Intellisense and I consider it a decent-sized hit to DX for all users to accommodate a rather niche problem. If you absolutely need proper input typing, you can still use the old approach:

const r = z.any().transform((val) => String(val));
type In = z.input<typeof r>;
type Out = z.output<typeof r>;

@aleclarson
Copy link

I don't like the idea of adding any generics to ZodString I'm afraid

Perhaps my approach is a good compromise then? …even though {coerce: true} is not supported, it enables the correct types when using the z.coerce API.

@colinhacks
Copy link
Owner

colinhacks commented Jan 30, 2023

That seems messy to me and not worth the headache with assignability and whatnot. I was aware of this issue before selecting the current approach and I still think its the best option from a set of imperfect options. The true solution is to rewrite Zod to avoid classes/inheritance but in the meantime, this is the best tradeoff. I'm gonna close, thanks to everyone for the ideas and discussion.

@aleclarson
Copy link

That seems messy to me and not worth the headache with assignability and whatnot.

  1. I don't know what you mean by "messy". Can you elaborate?

  2. As for the assignability, what are the practical use cases where that becomes an issue.

  3. Also worth bearing in mind that we can use any as the Input type in all cases if the assignability concerns are a deal breaker.

@RobinTail
Copy link
Contributor Author

The true solution is to rewrite Zod

Then why do you close the issue? @colinhacks

It is a confirmed bug that remains, right?
Your vision is a refactoring, not a patch. Ok.
Do you have a plan for that in a some upcoming future? Should this plan refer to this issue?
I don't understand.

@stale
Copy link

stale bot commented Apr 30, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-confirmed Bug report that is confirmed stale No activity in last 60 days
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants