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

Specific Optional Type #152

Closed
sylvanaar opened this issue Sep 15, 2020 · 8 comments
Closed

Specific Optional Type #152

sylvanaar opened this issue Sep 15, 2020 · 8 comments

Comments

@sylvanaar
Copy link
Contributor

sylvanaar commented Sep 15, 2020

I was just playing around with various things, and I noticed optional-ness is reapplied to things depending on how you write it, for example if I write it this way (see below), message is "double optional" since that is what the code says it is. But double optional doesn't have any meaning - so it can be flattened.

so i have this union which makes sense given the code below

message: z.ZodUnion<[z.ZodUnion<[z.ZodString, z.ZodUndefined]>, z.ZodUndefined]>;

can it become: message: z.ZodUnion<[z.ZodString, z.ZodUndefined]>;

Optional is not just a union with undefined though, it also includes "missing" / "not there at all". So there seems to be something that is not being captured currently.

Unions also seem to be a weak point in the error messaging which is otherwise top notch. Having a specific optional - lets you give the error message for the optional type instead of the union which i think is a pretty big deal.

Right now all unions (so all optionals) give the same error:

[Error: 1 validation issue(s)

     Issue #0: invalid_union at message
     Invalid input
   ]

vs a non-optional of the same

[Error: 1 validation issue(s)

     Issue #0: invalid_type at message
     Expected string, received number
   ]

This difference only make sense if you know that optional was being represented by a union with undefined in which we no longer know which option in the union is our type.

Code i was experimenting with is below for reference

const ErrorBase = z
    .object({
        message: z.string(),
        stack: z.string(),
    })
    .partial();

export const ErrorType = ErrorBase.extend({
    name: z.string(),
    code: z.number(),
    context: z.record(z.any()),
    cause: ErrorBase,
}).partial();
@sylvanaar
Copy link
Contributor Author

sylvanaar commented Sep 16, 2020

Will you accept a PR?

I have a branch with my work so far. master...sylvanaar:first-class-optionals

@colinhacks
Copy link
Owner

colinhacks commented Sep 16, 2020

Go ahead and submit the PR. My initial reaction to this concept was negative because I don't like the redundancy of representation. But I'm warming up to the idea. Your work on this looks excellent!

Edit: oops I see you've already submitted it 👍 I'm trying to publish the beta of Zod 2 today so I'm going to use your PR as a jumping off point.

@colinhacks
Copy link
Owner

colinhacks commented Sep 16, 2020

Optional is not just a union with undefined though, it also includes "missing" / "not there at all". So there seems to be something that is not being captured currently

Practically speaking how do you want Zod's behavior to change? Zod already handles this case correctly:

z.object({
  name: z.string().optional()
}).parse({}) // passes

// inferred type is { name?: string }
// note the question mark

@sylvanaar
Copy link
Contributor Author

sylvanaar commented Sep 17, 2020

Practically speaking how do you want Zod's behavior to change? Zod already handles this case correctly:

z.object({
  name: z.string().optional()
}).parse({}) // passes

// inferred type is { name?: string }
// note the question mark

First,. thank your considering my PR! Now - I think may seem like a minor detail. First - as you know Optional implies both missing and undefined in TS. Zod's type system only encodes the union with undefined, so it cannot currently express optionals - yet it is supported as you mention.

So, because of that it handles this case incorrectly.

z.object({
  name: z.string().or(z.undefined()),
}).parse({}); // passes

This is really a different type than the one in your example, and is inferred incorrectly as

const union: {
    name?: string | undefined;   // name: string | undefined     <-- is correct
}

Edit cool-frost-xfo3t

It may seem like a minor point - but I think a core project goal is to implement the Typescript type system. Typescript handles Type | undefined and Type? differently.

So can we agree I have translated the 2 zod examples above properly into TS below based on what the TS rules say?

type union = {
    name: string | undefined;
}

type optional = {
    name?: string,
} 

const union: union = {}  // error
const optional: optional = {}

Here is a TS Playground I made to illustrate a bit more: https://tsplay.dev/yNakpW

@colinhacks
Copy link
Owner

Gotcha. Unfortunately I prefer the current behavior for a couple reasons. If only ZodOptionals get the question mark, there are some unintuitive consequences. For instance, this schema would lack the question mark, because the name schema is a ZodUnion (the type returned from nullable). Most people, I think, want the question mark whenever undefined extends T where T is the inferred type of their schema. This is how Zod works currently.

const person = z.object({
  name: z.string().optional().nullable()
})

I understand the desire to have one-to-one correspondence with the TypeScript type system but I'd rather have an API that works as expected.

One potential workaround is to add a method that subtracts the question mark from the inferred type. This wouldn't affect the runtime behavior at all, just the static type.

const person = z.object({
  name: z.string().optional()
})
// => { name?: string | undefined }

person.noOptionals()
// => { name: string | undefined }

@colinhacks
Copy link
Owner

colinhacks commented Sep 17, 2020

Though this discussion is somewhat beside the point because this PR was already merged into Zod 2 🤙 I think having a dedicated ZodOptional class will make it easier to implement things like nested .pick() and a more robust .deepPartial() method.

But I still don't like the idea of changing Zod's current approach to optional typing in objects. It seems like almost no one needs { name: string | undefined } so it's not worth complicating Zod's behavior.

@sylvanaar
Copy link
Contributor Author

Though this discussion is somewhat beside the point because this PR was already merged into Zod 2 🤙 I think having a dedicated ZodOptional class will make it easier to implement things like nested .pick() and a more robust .deepPartial() method.

But I still don't like the idea of changing Zod's current approach to optional typing in objects. It seems like almost no one needs { name: string | undefined } so it's not worth complicating Zod's behavior.

Yes - that is really not an issue for me either in alot of cases there is no difference between ? and | undefined. Basically I saw this Optional as a key enabler for other things like you did.

I just wanted to be able to have optional parameters in functions so i didn't have to pass undefined for missing paramters and good error messages for optional types, and it seems like we are there. Thanks alot for all the hard work!

@colinhacks
Copy link
Owner

Thank you too! This saved me a lot of time. The clean optional error messages in Zod 2 are 🔥

optional parameters in functions

This isn't possible yet, like I mentioned in the other issue, but hopefully will be eventually.

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

2 participants