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

Fix optional error #37

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix optional error #37

wants to merge 1 commit into from

Conversation

unknownpgr
Copy link

I found an error where when using the default() method, a field that should not be optional becomes optional. z.isOptional() seems to indicate whether the input value is optional, not whether the output type is optional.

Capture

@sachinraja
Copy link
Owner

We need that check though. Checking if the node's name is ZodOptional doesn’t achieve much. It could still be optional but more deeply nested.

@simonkarman
Copy link

simonkarman commented Apr 2, 2024

The type that zod-to-ts outputs is what can be passed to the parse method of the schema.

See the following example:

const MySchema = z.object({ value: z.string() });
MySchema.parse({ value: 'abc' });
// MySchema.parse({}); // will fail

const MyDefaultSchema = z.object({ value: z.string().default('hello, world!') });
MyDefaultSchema.parse({ value: 'abc' });
MyDefaultSchema.parse({}); // will NOT fail

@unknownpgr Given this, your case should NOT be considered an error. Since you can successfully parse an object when the unOptional property is undefined, as it will be set to the default value by Zod.

@simonkarman
Copy link

@sachinraja Can you confirm this? (A) The type that zod-to-ts outputs is what can be passed to the parse method of the schema. (as oppose to (B) The type that zod-to-ts outputs is what comes out of the parse method of the schema)

If that (A) is confirmed, this would also mean the behaviour described by: #59 is actually not an issue and is intended.

In my situation (A) is actually exactly what I'm looking for, so it would be great if that's the case. However, I can see why (B) in some situation might be useful too.

Firstly: I think it needs to be clearly stated in the README or documentation which of the two it is.

Secondly: If there is merit for both, that both should have support.

@unknownpgr
Copy link
Author

@simonkarman Thanks for your explanation. According to your explanation, I mean (A), it is clear that my case is not an error and it is intended. But as you stated later, In my case I need (B).

I used this library to implement automatic API generation, something like openapi-typescript-codegen. So for me, the output type, which is used by business logic, is required. I expected that the type generated by zod-to-ts would be identical to the type z.infer<Schema>.

And if (A) is accepted, I think the generated type of any object schema should be Type & Record<string|number, any>, because If not explicitly set, the object schema would accept any arbitrary objects with required fields.

const s = z.object({});
s.parse({'a':'b'}); // Accepted. The result is `{}`

type T = {} // The type generated by zod-to-ts
const x:T = {'a':'b'}; // Can be accepted to schema s, but type error occurs.

@simonkarman
Copy link

simonkarman commented Apr 2, 2024

@unknownpgr thanks for pointing that out!

The Record<string|number, any> is redundant as in TypeScript an object is assignable to a type, even if it has properties unknown to the type (https://www.typescriptlang.org/docs/handbook/type-compatibility.html#starting-out). However, this is restricted when using a literal type in an assignment (https://www.typescriptlang.org/docs/handbook/2/objects.html#excess-property-checks). Just think about the following example:

const myMethod = (val: { x: number }) => console.info(val.x);
// myMethod({ x: 3, y: 3 }); // will fail
const someValueWithY = { x: 3, y: 3 };
myMethod(someValueWithY); // will NOT fail

However, it is important pointing out, as I'm not entirely sure in which ways Zod can handle unknown properties and whether these properties remain on the output of the parse method. Would have to dive deeper into this to see if that functionality would align between zod-to-ts and Zod given that (A) is true.

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

Successfully merging this pull request may close these issues.

3 participants