Skip to content

.optional() and --exactOptionalPropertyTypes #635

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

Open
bwbuchanan opened this issue Sep 6, 2021 · 34 comments
Open

.optional() and --exactOptionalPropertyTypes #635

bwbuchanan opened this issue Sep 6, 2021 · 34 comments
Labels

Comments

@bwbuchanan
Copy link

Currently, .optional() behaves like:

const user = z.object({
  username: z.string().optional(),
});
type C = z.infer<typeof C>; // { username?: string | undefined };

This results in a type mismatch when you're using --exactOptionalPropertyTypes in TypeScript 4.4 and expecting type C to match an interface definition like:

interface User {
  username?: string;
}

.partial(), .partialBy(), .deepPartial() all have the same issue.

It would be nice to unbundle the optionality of the key from the union type with undefined for the value.

I suggest that these methods be changed to specify the optional absence of the key by default, and perhaps accept an option to restore the old behavior of adding .or(z.undefined()) to the value schema(s). This would unfortunately be a breaking change, but it makes more sense than the current behavior, especially as more projects adopt --exactOptionalPropertyTypes.

@celmaun
Copy link

celmaun commented Sep 10, 2021

I soo want to start using exactOptionalPropertyTypes to put one more JavaScript The Bad Part(s)™️ behind. But our codebase uses Zod (❤️) heavily. Maybe I will take this on... @colinhacks Pull requests welcome? (As an opt-in feature until next major release?)

@scotttrinh
Copy link
Collaborator

I wonder if we can introduce this under a separate method like exactOptional? Or maybe have optional take a configuration parameter. If we go the config route, I'd want to get some direction from @colinhacks since there has been talks about using arguments to do things like custom error messages, etc, so we probably want to coordinate a bit.

@ViktorQvarfordt
Copy link

I don't think this there is any need for multiple methods for this, since if you don't have exactOptionalPropertyTypes enabled you can always assign undefined to an optional property, regardless of how it is typed.

Here is an example to just make all this explicit
image
If I disable the exactOptionalPropertyTypes TS option, then there are no errors. And of course, the bottom line is that we are missing an error for the line fooZod.a = undefined

@ukarlsson
Copy link

ukarlsson commented Oct 31, 2021

If we compare with io-ts, where everything seems to work with exactOptionalPropertyTypes, the behaviour is like this

To add undefined as part of a property, you use

t.type({
  v: t.union([t.undefined, t.string])
})

To make properties partial (i.e. with question mark), you use

t.partial({
  v: t.string
})

This leads to a very strict handling of undefined and question marks, which goes very well along with exactOptionalPropertyTypes.

It is important to note that when you use exactOptionalPropertyTypes you want good separation between undefined and question marks, so that if you want undefined, you get exactly undefined, and if you want question mark, you get exactly question mark - which it seems like io-ts is achieving, perhaps by accident, by using these somewhat more "primitive" constructs.

In Zod, we have .optional() which is backed by ZodOptional which you use like ZodOptional.create(z.string), and that basically adds undefined as part of the data type, so this is very similar to a construct like t.union([t.undefined, t.string]).

Then we have .partial() that you use on an object, and this actually just wraps the properties of the object in ZodOptional.

The addition of the question marks is done automatically by this code inside objectUtil that is employed by ZodObject to construct the output type

  type optionalKeys<T extends object> = {
    [k in keyof T]: undefined extends T[k] ? k : never;
  }[keyof T];

  type requiredKeys<T extends object> = Exclude<keyof T, optionalKeys<T>>;

  export type addQuestionMarks<T extends object> = {
    [k in optionalKeys<T>]?: T[k];
  } &
    { [k in requiredKeys<T>]: T[k] };

So the issue is that the question mark handling is currently inherently tied to whether the property type extends undefined, and in this case the question marks are added automatically.

@akomm
Copy link

akomm commented Nov 4, 2021

This might be drastically important when you work with document based databases, where a property might be a Dict of ID => VALUE

type ProductId = string & {readonly _: unique symbol}
type Entry = {id: ProductId; status: string}
type ProductsA = {[k in ProductId]: Entry}

declare const products: ProductsA
declare const productId: ProductId

// productA: Entry -> BAD, because products[productId] might be undefined
const productA = products[productId]

// Another approach, optional property
type ProductsB = {[k in ProductId]?: Entry}

declare const productsB: ProductsB

// product: Entry | undefined -> GOOD
const productB = productsB[productId]

// However, now we can:
productsB[productId] = undefined
// because without "exactOptionalPropertyTypes" enabled, {[k in ProductId]?: Entry} -> {[k in ProductId]?: Entry | undefined}

// also, given toArray conversion of a record:
type RecordValues<T> = T extends {[k: string | number | symbol]: infer V}
  ? V[]
  : never

// we get correctly, but unintentionally (Entry | undefined)[]
declare const arrayOfProductsB: RecordValues<ProductsB>

// "exactOptionalPropertyTypes" enabled behavior: don't append "undefined" to optional propertie's value type

@akomm akomm mentioned this issue Jan 3, 2022
@stale
Copy link

stale bot commented Mar 2, 2022

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.

@stale stale bot added the wontfix This will not be worked on label Mar 2, 2022
@ViktorQvarfordt
Copy link

I don't think this issue should be considered stale. It is a real issue that multiple users of the library are facing.

@stale stale bot removed the wontfix This will not be worked on label Mar 3, 2022
@vjau
Copy link

vjau commented Apr 20, 2022

Any progress on this ? Would you accept a pull request to add an exactOptional() for those that would want to use it without breaking existing code ?

@stale
Copy link

stale bot commented Jun 19, 2022

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.

@stale stale bot added the wontfix This will not be worked on label Jun 19, 2022
@akomm
Copy link

akomm commented Jun 19, 2022

Still relevant

@stale stale bot closed this as completed Jun 26, 2022
@tstehr
Copy link
Contributor

tstehr commented Jun 26, 2022

I don't think this should be closed due to staleness. This issue is currently blocking me from adopting --exactOptionalPropertyTypes and from the recent activity it looks like there are multiple users interested in a solution.

@akomm
Copy link

akomm commented Jul 12, 2022

Relevant issue just silently closed, regardless of activity, no word from author. Later can be understood if he has no time, but let bots close issues regardless of relevance and activity feels wrong.

@dilame
Copy link

dilame commented Dec 5, 2022

Another issue with this addQuestionMarks type – it looses the generic when use like this – the arbitrary field just becomes unexisted.

function dynamicZodObject<T>(t: T) {
  return z.object({
    arbitrary: z.any().refine((x): x is T => t),
  });
}

I spent so much time trying to fix it properly in zod to make a PR, but seems like it's just impossible. Right now i am fixing it like this

export type ZodObjectIdentity<T extends z.ZodRawShape, Side extends '_input' | '_output'> = {
  [K in keyof T]: T[K][Side];
};

export function zodObjectIdentity<T extends z.ZodRawShape>(
  t: T,
): z.ZodObject<T, 'strip', z.ZodTypeAny, ZodObjectIdentity<T, '_output'>, ZodObjectIdentity<T, '_input'>> {
  // @ts-ignore
  return z.object(t);
}

So, despite the zod is a great library, addQuestionMarks is the most bad design decision in zod.
First of all – type: string | undefined is not the same as type?: string | undefined. Zod erases these boundaries, which is contradicts TypeScript-first schema validation library.
And another issue – it looses generics, which is also a huge pitfall when we talk about Typescript-first
In that case i would really prefer more-verbose io-ts decision with intersection([type(...), partial(...)])

@jameshfisher
Copy link

(Moving my complaints in #1540 and #1510 here, because it sounds like the same issue.)

My specific problem is that I'm trying to parse values into this type:

type FormattedText = { text: string; bold?: true; };

The bold flag, if present, is always the value true. Example test cases:

// Valid values include
{ text: "foo" }
{ text: "foo", bold: true }

// Invalid values include
{ text: "foo", bold: false }
{ text: "foo", bold: undefined }

This schema is designed to work with Slate, and the values are in a database. The design is therefore non-negotiable.

I can't figure out how to configure zod to make the bold property optional, without also adding undefined to the type of the property. The .optional() method seems to conflate the two: it makes the property optional, but also allows the value undefined if present. For example, zod.parse allows the value { text: "foo", bold: undefined }.

I realize I could do this with .refine(), but then the constraint is not represented in the type system. I need the type of .parse() to be { text: string; bold?: true; }.

This issue is blocking me from using zod, because I can't find any reasonable workaround for it.

I understand that zod is designed around an assumed TypeScript config that its users are supposed to set. This makes sense: you can't design for all $2^{\text{flags}}$ TypeScript variants. Adding a new function or new import goes down a dangerous path, e.g. import ... from 'zod/exact_optional_properties_but_no_strict_null_checks_or_checked_index_access' .

But is it documented somewhere what TypeScript config zod is designed for, and why? As a design principle, I would design zod around the "strictest" form of TypeScript, i.e. for users that are using TypeScript effectively. So is there a particular reason that zod is designed for users with exactOptionalPropertyTypes turned off?

@akomm
Copy link

akomm commented Jan 26, 2023

@jameshfisher
I agree with most points here. I would like to agree on all, but there are maybe some conflicts I will explain further that we need to pay attention to. No separate namespaces: agree. Max possible strictness: agree. So where's the problem?

Well, exactOptionalPropertyTypes isn't part of strict for a reason. There is also the noUncheckedIndexedAccess option and others might come later. In the following example, we can see how the later partially takes away some benefits of the former.

See this in TS playground and toggle noUncheckedIndexedAccess.

// exactOptionalPropertyTypes always ON

type UserIdBrand = {readonly UserId: unique symbol}
type UserId = string & UserIdBrand

type User = {
  id: UserId
  name: string
}

declare const id: UserId
declare const r1: Record<UserId, User>
declare const r2: {[_ in UserId]: User}

// noUncheckedIndexedAccess "1" => u1 & u2: User | undefined (bad)
// noUncheckedIndexedAccess "0" => u1 & u2: User (good)
const u1 = r1[id]
const u2 = r2[id]

declare const a1: User[]
declare const t1: [User, User]

// noUncheckedIndexedAccess "1" => u3: User | undefined (good), u4: User (good)
// noUncheckedIndexedAccess "0" => u3: User (bad), u4: User (good)
const u3 = a1[0]
const u4 = t1[0]

What IndexAccess does could have been solved with exactOptional too, but it would have required an alternation to existing types, including Array, Record and more. I think the TS devs didn't want to introduce such a fundamental break in the entire core. The implication would require an entire separate discussion.

So we have this two features that didn't made it cleanly into the strict because of certain unsoundness left for the explained reasons, probably among others I can guess but let's not deviate off to much.

With exactOptional we have also some problems. The runtime behavior can not be enforced or guaranteed, even if you have it enabled. You might convert a Record<UserId, User> to Array<User> and Record<UserId, User | undefined> to Array<User | undefined>, but at runtime, you could still get an undefined value, but your code didn't test for this case. Now you will try to access properties on undefined when rendering, etc.

However, the changes made in zod have not improved, but worsen the situation as I've explained already.

This is why I think we need a specific API with type and runtime behavior guaranteed.

  1. optional() -> produces ?: T | undefined and will allow undefined at runtime
  2. optionalExact (or exactOptional or optional({exact: true})) -> produces ?: T and will fail parsing undefined value.

The 2. will however have the | undefined added when exactOptional is disabled.

@stale
Copy link

stale bot commented Apr 26, 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.

@stale stale bot added the stale No activity in last 60 days label Apr 26, 2023
@scotttrinh scotttrinh added breaking-change Might be unintentional not-intuitive-behavior and removed stale No activity in last 60 days labels Apr 26, 2023
@stale
Copy link

stale bot commented Jul 25, 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.

@stale stale bot added the stale No activity in last 60 days label Jul 25, 2023
@luxaritas
Copy link

Still relevant

@stale stale bot removed the stale No activity in last 60 days label Jul 25, 2023
@yayza
Copy link

yayza commented Aug 14, 2023

I can't figure out how to configure zod to make the bold property optional, without also adding undefined to the type of the property. The .optional() method seems to conflate the two: it makes the property optional, but also allows the value undefined if present. For example, zod.parse allows the value { text: "foo", bold: undefined }.

this worked for me: #2314 (comment)

EDIT: Nvm, after using the .parse() method it makes the validated data's properties back to type | undefined

@bazuka5801
Copy link

Still relevant

@bazuka5801
Copy link

So many reactions! 🔥
So I wrote a workaround, check the PR at the link above.

This patch can be installed for testing by adding the following lines to package.json.

"zod": "npm:@bazuka5801/[email protected]"

⚠️ Be careful and careful, this patch may break your code.

@sweetpalma
Copy link

@bazuka5801 thanks, this is very helpful! Would be great to see this approved & merged...

@alvaro-cuesta
Copy link

alvaro-cuesta commented Nov 22, 2023

Note this has actual runtime implications, e.g. 'a' in {} vs 'a' in { a: undefined }. Also Object.keys et al.

This is currently impacting me too.

Also something to consider (I'm not sure if this was brought up above) is whether...

z.object({ a: z.string().exactOptional() }).safeParse({ a: undefined })
z.object({ a: z.string() }).exactPartial().safeParse({ a: undefined })

...should fail to parse or instead parse successfully but omit the key (like JSON serialization behavior), i.e. a way to affect both Input and Output separately. I'm just thinking out loud here, but it might be nice if this was an option in exactOptional or maybe a different method so you could choose both behaviors (both of which could make sense in different scenarios?) Or can this behavior be simulated with current .optional()/.partial() + some transform?

EDIT: I think my last paragraph can be simulated like this.

type OmitUndefined<Object, Keys> = {
  [K in keyof Object]: K extends Keys
    ? Exclude<Object[K], undefined>
    : Object[K];
};

/**
 * Zod's `optional`, `partial`, etc. generate a type of `{ key?: SomeType | undefined }` but, since
 * we are using `exactOptionalPropertyTypes` in tsconfig, we want `key?: SomeType`.
 *
 * This function takes an object and removes the `undefined` from some optional properties.
 *
 * See https://github.com/colinhacks/zod/issues/635
 */
export function transformOmitPartial<
  Object extends object,
  Keys extends keyof Object,
>(keysToOmit: Array<Keys>): (value: Object) => OmitUndefined<Object, Keys>;
export function transformOmitPartial<Object extends object>(): (
  value: Object,
) => OmitUndefined<Object, keyof Object>;
export function transformOmitPartial<
  Object extends object,
  Keys extends keyof Object,
>(keysToOmit?: Array<Keys>): (value: Object) => OmitUndefined<Object, Keys> {
  return (value: Object) => {
    const realKeysToOmit =
      keysToOmit ?? (Object.keys(value) as Array<keyof Object>);

    for (const key of realKeysToOmit) {
      if (key in value && typeof value[key] === undefined) {
        delete value[key];
      }
    }

    return value as OmitUndefined<Object, Keys>;
  };
}

Not great because after .transform() you cannot .extend() for example, but it's... something.

@dkrieger
Copy link

fwiw I would happily suffer through a major version bump to get rid of the optional key magic for any and unknown, as it's a non-trivial blocker to matching a pre-existing typescript type

@dkrieger
Copy link

API-wise, rather than adding some new type, this should be behavior you can configure when initializing zod, e.g. {addQuestionMarks: false}. any, unknown, etc should not be made optional when that configuration is made. this will break exactly 0 existing use cases.

@benhickson
Copy link

benhickson commented May 15, 2024

i just want to call out that there is a footgun for consumers of zod types that needs to be avoided:

let's say i have a type for email recipients, consisting of the email address and an optional name.

interface EmailRecipient {
  email: string;
  name: string | undefined;
}

when users try to pass just an email, they are prompted by typescript to either pass a name or explicitly pass undefined. great.

when zod renders the type with a question mark,

interface EmailRecipient {
  email: string;
  name?: string | undefined;
}

a consumer can pass the email, typescript passes, and they have no idea that it's possible to pass a name. so there can be places littered around a codebase with inconsistent usage of the that name field. now the debugging developer has to trace every spot manually to figure out why the name isn't showing up.

this is exactly the sort of problem that typescript is supposed to help us avoid.

@NorthBlue333
Copy link

NorthBlue333 commented Jun 4, 2024

@benhickson Just wanted to add that javascript CAN determine the difference at runtime, as it has been said by others, stated by @alvaro-cuesta i.e. And it definitely has runtime impact for libraries which treat them differently!
A simple sample of code can demonstrate that:

const foo = {a: undefined};
// undefined
const bar = {};
// undefined
console.log(foo.a)
// undefined
console.log(bar.b)
// undefined
console.log(Object.hasOwn(foo, 'a'))
// true
console.log(Object.hasOwn(bar, 'a'))
// false

Then a logic as the following would be all wrong without the option to have exact optional properties (with runtime checks):

// obj defines properties to be set in database
function dbInsert(obj: Partial<Document>) {
  const documentInsert = {};
  for (const property in obj)
  {
    documentInsert[property] = obj[property];
  }
  database.insert(documentInsert);
  // error: field value cannot be undefined!
}

@benhickson
Copy link

Thanks @NorthBlue333 , I've edited my comment. I think that makes it even more important to fix!

@igniscyan
Copy link

Running into this exact issue using the drizzle-zod library. After a couple of wasted hours, discovered the cause...

We're approaching 3 (!) years since this issue was first brought up, does anyone have proposals for how to resolve this?

@eikowagenknecht
Copy link

For anyone not having checked out all the other links: According to #2675 this is on the roadmap for ZOD 4 and a RFC will be published "soon".

@gabe-cc
Copy link

gabe-cc commented Nov 25, 2024

I am using this hack in the meanwhile:

export const mutateStrictOptionalFields = <
  T extends z.ZodObject<any>,
  K extends keyof T['shape'],
>(schema: T, keys: Array<K>)
: z.ZodObject<
  Omit<T['shape'], K> & { [k in K]? : T['shape'][k]} ,
  T['_def']['unknownKeys'] ,
  T['_def']['catchall'] ,
  {
    [k in keyof T['shape'] as Exclude<k, K>]: T['shape'][k]['_output'];
  } & {
    [k in K]?: T['shape'][k]['_output'];
  },
  {
    [k in keyof T['shape'] as Exclude<k, K>]: T['shape'][k]['_input'];
  } & {
    [k in K]?: T['shape'][k]['_input'];
  }
> => {
  for (const key of keys) {
    schema.shape[key] = schema.shape[key].optional() ;
  }
  // @ts-expect-error
  return schema.refine(arg => keys.every(key => !(key in arg) || arg[key] !== undefined)) ;
}

I do not know how reliable it is, but it passes tests + type-checks so far.

@santiago-elustondo
Copy link

For anyone not having checked out all the other links: According to #2675 this is on the roadmap for ZOD 4 and a RFC will be published "soon".

PR was closed without merge, there doesn't seem to be a proper solution for this issue yet

@santiago-elustondo
Copy link

my current workaround is enforcing a no-optional-properties rule by convention/lint, which then allows me to use .nullable() instead of .optional(), which is much more predictable and db-friendly.

@igniscyan
Copy link

my current workaround is enforcing a no-optional-properties rule by convention/lint, which then allows me to use .nullable() instead of .optional(), which is much more predictable and db-friendly.

I honestly never thought about it this way... thank you for the aha moment, on a Friday nonetheless!

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

Successfully merging a pull request may close this issue.