colinhacks / zod

TypeScript-first schema validation with static type inference
https://zod.dev
MIT License
33.09k stars 1.15k forks source link

optional should not add undefined to type signature #3186

Open hesxenon opened 7 months ago

hesxenon commented 7 months ago

I'm sorry if this is a duplicate, I could not find an existing issue or discussion that seemed to be exactly about the following. It's really rather simple: the following schema infers the wrong type

const schema = z.object({
  foo: z.string().optional()
});

is inferred as

type Schema = {
  foo?: string | undefined
};

when it should really be

type Schema = {
  foo?: string
}

Let me showcase the issue with two different typescript playgrounds. They both have the same content, but the second has exactOptionalPropertyTypes enabled.

exactOptionalPropertyTypes: false

This playground errors, because even though we know that foo exists on result the compiler adds undefined to the set of possible types for result.foo.

exactOptionalPropertyTypes: true

And here we eliminated the error, because the compiler knows that if foo exists on result it must be exactly a string.

The problem to note here is that the decision to include undefined should be up to the compiler. Zod takes that decision away and subsequently makes it impossible to truly express optional keys.

TahsinAyman commented 7 months ago

ill try to fix this issue please

JacobWeisenburger commented 7 months ago

image

TahsinAyman commented 7 months ago

thanks for explaining let me fix it it would be appreciated if i were assigned in this task

hesxenon commented 7 months ago

@JacobWeisenburger I'm not sure I follow, what's your point here?

It seems you've disabled exactOptionalPropertyTypes in your screenshot, hence the compiler adds undefined, but other than that I don't see what you're getting at :/

JacobWeisenburger commented 7 months ago

Here's my tsconfig.json. It doesn't have exactOptionalPropertyTypes specified.

{
    "compilerOptions": {
        "lib": [
            "dom",
            "dom.iterable",
            "esnext"
        ],
        "module": "esnext",
        "target": "esnext",
        "moduleResolution": "bundler",
        "moduleDetection": "force",
        "allowImportingTsExtensions": true,
        "noEmit": true,
        "composite": true,
        "strict": true,
        "downlevelIteration": true,
        "skipLibCheck": true,
        "jsx": "preserve",
        "allowSyntheticDefaultImports": true,
        "forceConsistentCasingInFileNames": true,
        "allowJs": true,
        "types": [
            "bun-types"
        ]
    },
    "include": [
        "src/**/*",
        "package.json"
    ]
}
hesxenon commented 7 months ago

yeah? The default is false; that doesn't make it any more correct for zod to take away the decision from the compiler for those that have it set to true.

Imho this isn't an enhancement but a bug - zod "deliberately" add's implicit nullability even if that nullability is explicitly forbidden. From what I'm reading from other issues in this repo I'm fairly certain that this is not what zod wanted or intended.


fyi: @types/bun exists by now, just in case.

TahsinAyman commented 7 months ago

i understood the problem and i also looked after the source code and understood what's wrong let me try to fix it

hesxenon commented 7 months ago

Who exactly are you asking for permission? It's github, just create a PR?

On Mon, Jan 29, 2024, 03:03 Tahsin Ayman @.***> wrote:

i understood the problem and i also looked after the source code and understood what's wrong let me try to fix it

β€” Reply to this email directly, view it on GitHub https://github.com/colinhacks/zod/issues/3186#issuecomment-1913839294, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACOHZTWSPGHW7H5UKTYYV5DYQ37NHAVCNFSM6AAAAABCJTIF62VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJTHAZTSMRZGQ . You are receiving this because you authored the thread.Message ID: @.***>

TahsinAyman commented 7 months ago

sorry im just a beginner.

hesxenon commented 7 months ago

the more I look at this, the more it looks like this won't be possible without a breaking change. While I did get correct optional inference working (basically just switch addQuestionMarks and baseObjectOutputType and infer Required Key with extends ZodOptional) the core of the problem is that ZodOptional adds |undefined to its in and output.

Removing that assumption breaks other tests because optional can't depend on its wrapping context.

E.g. here

const stringSchema = z.string().optional();

const objectSchema = z.object({
  foo: stringSchema
});

type S = z.infer<typeof stringSchema> // expectation: string | undefined
type O = z.infer<typeof objectSchema> // expectation: { foo?: string }

Since the stringschema can't know when it's being used in an object context it's impossible to correctly represent optional keys without also making the value "undefinedable".

Imho the only true solution (and maybe even the right one) would be to strictly reserve .optional() for usage within objects and to have a new operation like .nullable(), e.g. .orUndefined() for everything else.

@colinhacks , sorry for directly mentioning you but do you see any other solution?

TahsinAyman commented 7 months ago

πŸ”₯ I also tried but it didn't seem to make any sense to me πŸ‘ŽπŸ»

TahsinAyman commented 7 months ago

what i have come to notice when contributing is the ZodOptional is the one which adds | undefined on the T["output"]

class ZodOptional<T extends ZodTypeAny> extends ZodType<
  T["_output"] | undefined,
  ZodOptionalDef<T>,
  T["_input"] | undefined
>

if you remove the | undefined from here it seems to remove it but removes the ? from the key which dosent really make sense to me what so ever.

TahsinAyman commented 7 months ago

@colinhacks nothing seems to be working. and is there any source code docs. and there is no comment / docs in the codebase!!! πŸ‘ŽπŸ»

hesxenon commented 7 months ago

@TahsinAyman it's their prerogative not to consider this issue and they're under no obligation to write the codebase in any other way than it is now.

Honestly, if zod were my project I'd close this issue since

  1. considering the current structure and maturity of this project it just doesn't make sense to introduce a breaking change for an edge case (even though it's a valid case)
  2. fixing it without introducing a breaking change to the api would require an enormous rewrite

If people then started to complain about how my codebase doesn't cater to somebody elses taste I'd be less than willing to see their point.

For me it just meant sticking closer to FP patterns and using it like this:

z.object({
  foo: z.string().optional().transform(option.fromNullable)
})
sp88011 commented 6 months ago

@hesxenon can you expand on your workaround please? Where does fromNullable come from?

hesxenon commented 6 months ago

@sp88011 it comes from fp-ts' implementation of the option monad.

Since ZodObject makes each key optional where the value contains undefined (which is wrong) all you have to do is transform the value to something that does not include undefined after the optional codec.

EphremDemlew commented 3 months ago

I found a work arround in whitch you can make it a nullable too and when the type is infered the null is added as a type incase thats what you nedded

profileImage: z.string().optional().nullable(),

fuadsaud commented 1 month ago

I have been bitten by this problem recently. Here's a simplified version of the use case I have:

type Entity = {
  name: string;
  description?: string;
};

const EntityCreationRequest = z.object({
  name: z.string(),
  description: z.string().optional(),
});

const entity: Entity = EntityCreationRequest.parse({
    name: "hi there",
});

The parse call above issues the following compiler error:

'entity' is declared but its value is never read.typescript(6133)
Type '{ name: string; description?: string | undefined; }' is not assignable to type 'Entity' with 'exactOptionalPropertyTypes: true'. Consider adding 'undefined' to the types of the target's properties.
  Types of property 'description' are incompatible.
    Type 'string | undefined' is not assignable to type 'string'.
      Type 'undefined' is not assignable to type 'string'.typescript(2375)

This is because the schema accepts a present, undefined description while my Entity type doesn't.

On one hand, Zod's notion of optionality belongs to the schema of the value itself, whereas the optionality in typescript is in the relationship between an object key and a value, so the current behavior doesn't seem completely wrong to me in this specific case, but it does feel wrong when applying partial or deepPartial to an object schema.

I guess the only non-breaking fix would be to introduce a new type of optionality, attached to the object key itself and have that behave differently, which would probably require changes to partial and deepPartial as well.

Unfortunately I'm not aware of a workaround at this moment.

MaLiN2223 commented 2 weeks ago

I have been bitten by this problem recently. Here's a simplified version of the use case I have:

type Entity = {
  name: string;
  description?: string;
};

const EntityCreationRequest = z.object({
  name: z.string(),
  description: z.string().optional(),
});

const entity: Entity = EntityCreationRequest.parse({
    name: "hi there",
});

The parse call above issues the following compiler error:

'entity' is declared but its value is never read.typescript(6133)
Type '{ name: string; description?: string | undefined; }' is not assignable to type 'Entity' with 'exactOptionalPropertyTypes: true'. Consider adding 'undefined' to the types of the target's properties.
  Types of property 'description' are incompatible.
    Type 'string | undefined' is not assignable to type 'string'.
      Type 'undefined' is not assignable to type 'string'.typescript(2375)

This is because the schema accepts a present, undefined description while my Entity type doesn't.

On one hand, Zod's notion of optionality belongs to the schema of the value itself, whereas the optionality in typescript is in the relationship between an object key and a value, so the current behavior doesn't seem completely wrong to me in this specific case, but it does feel wrong when applying partial or deepPartial to an object schema.

I guess the only non-breaking fix would be to introduce a new type of optionality, attached to the object key itself and have that behave differently, which would probably require changes to partial and deepPartial as well.

Unfortunately I'm not aware of a workaround at this moment.

I encountered the same, did you manage to figure out how to get it fixed?