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

feat: [#1075] Allows more discriminatedUnion values, use DiscriminatedValue as union element #1589

Conversation

maxArturo
Copy link
Contributor

@maxArturo maxArturo commented Nov 23, 2022

Partially closes #1075 , fully closes #1884 with the following:

feature: more valid discriminator values

The following are now well-supported discriminator values (enum/undefined could have been already)

  • z.undefined()
  • z.null()
  • z.enum()
  • z.discriminatedUnion()

This does not address functionality like the below, where a full primitive validator (like z.string()) is used as discriminator. This requires more changes and I'd like to address it separately.

  const ResultSchema = z.discriminatedUnion("error", [
    z.object({ error: z.string().min(1) }), // error here
    z.object({ error: z.undefined(), value: z.string().min(1) }),
  ]);

feature: use ZodDiscriminatedUnion as a union element

Allows adding a ZodDiscriminatedUnion as an element in the discriminated union, such that this is possible:

  const A = z.object({ type: z.literal("a") });
  const B = z.object({ type: z.literal("b") });
  const C = z.object({ type: z.literal("c").optional(), c: z.literal(true) });
  const AorB = z.discriminatedUnion("type", [A, B]);
  // using another discriminated union with the same discriminator as a union member
  const AorBorC = z.discriminatedUnion("type", [AorB, C]);
  type full = z.infer<typeof AorBorC>;
  /**
   * 
type full = {
    type: "a";
} | {
    type: "b";
} | {
    type?: "c" | undefined;
    c: true;
}
   */

Benchmarks for the new functionality (compared to two and four union elements, where nested is one of the options of DiscriminatedUnion itself):

$ yarn benchmark                                                                                                                                                                                           
yarn run v1.22.19
$ tsx src/benchmarks/index.ts
z.discriminatedUnion: double: valid: a x 994,112 ops/sec ±0.91% (85 runs sampled)
z.discriminatedUnion: double: valid: b x 964,468 ops/sec ±0.98% (85 runs sampled)
z.discriminatedUnion: double: invalid: null x 31,516 ops/sec ±1.10% (80 runs sampled)
z.discriminatedUnion: double: invalid: wrong shape x 34,323 ops/sec ±1.37% (81 runs sampled)
z.discriminatedUnion: many: valid: a x 948,977 ops/sec ±0.89% (87 runs sampled)
z.discriminatedUnion: many: valid: c x 944,571 ops/sec ±0.85% (85 runs sampled)
z.discriminatedUnion: many: invalid: null x 29,904 ops/sec ±1.24% (87 runs sampled)
z.discriminatedUnion: many: invalid: wrong shape x 32,622 ops/sec ±1.16% (88 runs sampled)
z.discriminatedUnion: nested: valid: a x 961,652 ops/sec ±0.82% (85 runs sampled)
z.discriminatedUnion: nested: valid: b x 963,830 ops/sec ±0.96% (86 runs sampled)
z.discriminatedUnion: nested: valid: c x 956,284 ops/sec ±0.88% (84 runs sampled)
z.discriminatedUnion: nested: invalid: null x 29,937 ops/sec ±1.19% (87 runs sampled)
z.discriminatedUnion: nested: invalid: wrong shape x 964,482 ops/sec ±0.91% (86 runs sampled)
✨  Done in 71.60s.

feature: disjointed discriminators for nested DUs

Now the following is possible:

  const subtype = z.discriminatedUnion("subtype", [
    z.object({
      type: z.literal("baz"),
      subtype: z.literal("able"),
    }),
    z.object({
      type: z.literal("bauble"),
      subtype: z.literal("beehive"),
      undertype: z.literal("alpha"),
    }),
    z.object({
      type: z.literal("baz"),
      subtype: z.literal("baker"),
    }),
  ]);

  const schema = z.discriminatedUnion("type", [
    z.object({
      type: z.literal("foo"),
    }),
    z.object({
      type: z.literal("bar"),
    }),
    subtype,
  ]);

/**
type schema = {
    type: "baz";
    subtype: "able";
} | {
    type: "bauble";
    subtype: "beehive";
    undertype: "alpha";
} | {
    type: "baz";
    subtype: "baker";
} | {
    type: "foo";
} | {
    type: "bar";
}
*/

Care was taken to ensure that there aren't any edge cases where nested DUs can be created without overlapping, resolvable discriminators at every level.

Follow-ups

  • Use the following zod validators as DiscriminatedUnion elements:
    • z.intersection()
    • z.string()
    • z.boolean()
    • others depending on implementation

@netlify
Copy link

netlify bot commented Nov 23, 2022

Deploy Preview for guileless-rolypoly-866f8a ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 0f16bf7
🔍 Latest deploy log https://app.netlify.com/sites/guileless-rolypoly-866f8a/deploys/63c5533e30f0ac00075cfbe7
😎 Deploy Preview https://deploy-preview-1589--guileless-rolypoly-866f8a.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@maxArturo maxArturo force-pushed the feat/1075-discriminated-unions-more-types branch from 132467c to d1aae88 Compare November 23, 2022 17:39
@maxArturo maxArturo marked this pull request as ready for review November 23, 2022 20:08
@maxArturo maxArturo force-pushed the feat/1075-discriminated-unions-more-types branch from 0503ebf to 2e60e27 Compare November 23, 2022 20:12
@maxArturo maxArturo changed the title (WIP) feat: [#1075] Allows more discriminatedUnion values, use DiscriminatedValue as union element feat: [#1075] Allows more discriminatedUnion values, use DiscriminatedValue as union element Nov 23, 2022
@maxArturo maxArturo force-pushed the feat/1075-discriminated-unions-more-types branch 2 times, most recently from 9ad0944 to 7d0c8b9 Compare December 9, 2022 17:10
Also adds functionality for disjointed nested discriminators,
optional types and other literals. Updates README.md and tests.
@maxArturo maxArturo force-pushed the feat/1075-discriminated-unions-more-types branch from 7d0c8b9 to 021ca61 Compare December 19, 2022 13:12
@hornta
Copy link

hornta commented Jan 2, 2023

Is this PR waiting for a review? I'm looking forward to this being merged.

@maxArturo
Copy link
Contributor Author

@JacobWeisenburger do you have approval privileges? Do you mind looking at this? Thanks!

@JacobWeisenburger
Copy link
Contributor

@JacobWeisenburger do you have approval privileges? Do you mind looking at this? Thanks!

I can approve things, but I'm not sure I have enough experience to approve this. I really like the idea, I'm just not sure if there are unintended consequences.

@maxArturo
Copy link
Contributor Author

@JacobWeisenburger understood! I tried to be thorough with the tests to catch any edge cases I could think of. With that said - this is additive so if anything, it should hopefully not impact any existing functionality. Can you think of anything else we can look at in order to merge?

Maybe @colinhacks can chime in!

src/types.ts Outdated Show resolved Hide resolved
@Fuzzyma
Copy link

Fuzzyma commented Jan 14, 2023

Hey @maxArturo can I be of any help getting this pr landed? I am looking for a solution to nested discriminated unions for month already and your pr looks like the most complete effort :)

@maxArturo
Copy link
Contributor Author

@Fuzzyma I just have one PR comment to resolve which should happen today or tomorrow (traveling ATM). Otherwise it should be good to go.

The main concern is getting someone with merger role to approve or give FB; @scotttrinh @colinhacks anyone else come to mind @JacobWeisenburger ?

@Fuzzyma
Copy link

Fuzzyma commented Jan 14, 2023

Sounds awesome! Hope your notebook battery doesn't die and you can work 😁

Maybe their discord server has anyone who can approve?

@maxArturo maxArturo requested review from teunmooij and removed request for scotttrinh and colinhacks January 16, 2023 13:39
@maxArturo
Copy link
Contributor Author

OK! hopefully more 👀 on this can get it to the finish line. Let me know what else I can do to get this approved!

@Fuzzyma
Copy link

Fuzzyma commented Jan 23, 2023

Sooo, who is allowed to merge this now? :D

@maxArturo
Copy link
Contributor Author

@Fuzzyma I think Scott and Colin. Here's what I got from @scotttrinh late last week on discord:

I don't have a ton of time these days to weigh in on changes like this, tbh. I can try to take some time soon, but honestly we've fallen back to just waiting for Colin to have time to swing around and do a big round of reviews and merging.

I think until then, we'll have to sit tight and wait for these to get pulled into actual package releases. Also, if you're desperate for this you can always install directly from a github repo into your package.json:

git+https://<github>/<org or person>/<repo>.git#<branch>

@Fuzzyma
Copy link

Fuzzyma commented Jan 24, 2023

@maxArturo that is actually what I ended up doing end I found that it doesn't work because the dist files are (rightfully) not in the repo. But you can get around that by adding a prepack script that builds the package. So I had to fork the fork to make that change and install from my fork :D

Also: this package gets more and more traction nowadays (because it's awesome!) and the prs will only get more.
Maybe more people should get merge rights (after proving their capabilities). Any plans on this?

@helmturner
Copy link

@maxArturo that is actually what I ended up doing end I found that it doesn't work because the dist files are (rightfully) not in the repo. But you can get around that by adding a prepack script that builds the package. So I had to fork the fork to make that change and install from my fork :D

Also: this package gets more and more traction nowadays (because it's awesome!) and the prs will only get more. Maybe more people should get merge rights (after proving their capabilities). Any plans on this?

I second this... Zod is an amazing contribution to the TS community, and there are a lot of excellent contributions in the pipeline. It would be a shame to see that pipeline bottlenecked by a busy BDFL.

@peterfoldes
Copy link

This is an amazing PR, just what I would need. Looking forward to this making into a release! 👍

@hornta
Copy link

hornta commented Feb 16, 2023

@joseph-lozano Do you want to take a look at this and maybe have it merged? @maxArturo There are some conflicts in the branch.

@teunmooij
Copy link

Wow this is taking long the get merged...

@scotttrinh
Copy link
Collaborator

Hey friends and community members, just wanted to check in here with the latest. Thanks so much to everyone's hard work for getting this PR in shape, it's well done and I appreciate that lots of people from across the community have contributed time and thought to it. Sincerely, thanks!

Aside from my own availability for staying on top of reviews and PRs here, I've been a little shy about building atop the existing discriminatedUnion work without getting more alignment and direction from @colinhacks . Since the appearance of this feature (which I helped merge without a lot of alignment 😅 ), he's expressed feelings that there should be a better way to achieve this with a different API. Turns out he has had a chance to take a swing at that in #2106 and I think we're unlikely to merge any improvements to discriminatedUnion until we get a more firm resolution there.

Apologies for not addressing these dynamics sooner, and I hope people have been able to find ways around the current limitations or maybe used their own fork while the dust is (slowly) settling on this. discriminatedUnion was originally merged as a "better than nothing" solution to the most simple cases, so one workaround to any issues is to use a bare union which should work for any cases we're not explicitly handling here. I know the error reporting is really noisy for union though, so I sympathize with wanting something better here.

Thanks for your patience and understanding! Let's keep this open and we'll continue to gather feedback on the proposed changes in #2106 and leave the discussion here for technical or design questions about this change specifically.

@scotttrinh
Copy link
Collaborator

I think there is a separate issue that's been raised here about governance and how we maintain such a popular library, and I'd welcome a lively discussion about this over on the Discord if anyone has the inclination or experience to help us out with that.

@tianhuil
Copy link

Would love to see this @maxArturo!

@stale
Copy link

stale bot commented Jul 23, 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 23, 2023
@maxArturo
Copy link
Contributor Author

Hey yall. Sorry for having been AFK all these months - lots of personal things were happening.

In short - I'm either happy to cleanup/fix this PR or close it, depending on whatever happens with #2106. Since we'd just need this functionality to be integrated in whatever way the repo owners seem fit, we're basically on a holding pattern there.

I will say too - a bit of the slowness on these overarching API decisions did cause me a bit to lose focus and switch gears on other tasks. I figure that's the case with other contributors too. But that's another story and no justification to leave people hanging, so I apologize again.

There's also https://github.com/teunmooij/zod-discriminated-union which @teunmooij put together in case you need this now 😄.

@stale stale bot removed the stale No activity in last 60 days label Jul 26, 2023
@colinhacks
Copy link
Owner

colinhacks commented Mar 21, 2024

I appreciate the work done for this PR by Arturo and all the good discussions. Apologies for the delay on replying. If you've read my opinion of ZodDiscriminatedUnion on other issues, you may understand why I wasn't very responsive in addressing this PR.


The first comment in this thread is a little out of date, as all of the below have supported as discriminators for a long time now:

  • z.undefined()
  • z.null()
  • z.enum()
  • z.nativeEnum()

Other PRs were merged around the time this one was created, which added support for these things. This PR adds support for ZodOptional as well, which I think is good, but perhaps not a highly demanded feature.

The main thrust of this PR is the ability to nest ZodDiscriminatedUnions.

My position on this is that it adds to much implementational complexity relative to the value of this API. It's a very niche feature that would increase Zod's unzipped bundle size by ~2kb. Out of the gate, that isn't worth it imo.

On top of that, this can be achieved quite simply with .options and destructuring. I've just documented this pattern in the README.

const A = z.discriminatedUnion([ ... ]);
const B = z.discriminatedUnion([ ...A.options, ...more options])

On top of that ZodDiscriminatedUnion will probably not exist in its current form in the next major version of Zod for the reasons described here.

All together, I wasn't excited about this idea. Even regardless of bundle size considerations, I'm getting increasingly allergic to this kind of visitor-pattern implementation seen in getDiscriminator. (This is why things like deepPartial were deprecated.) It's complex, edge-casey, and means Zod's parsing logic doesn't integrate with custom subclasses of ZodType.


I've just merged this PR that adds support for more types (but not ZodDiscriminatedUnion): #3346

@maxArturo I really appreciate the work you put into this. It's a beautiful approach. 🙌

@colinhacks colinhacks closed this Mar 21, 2024
@maxArturo
Copy link
Contributor Author

Thanks @colinhacks for taking the time to explain your reasoning. That makes sense to me and I think it’s a good and clean API.

Given that it’s one of my first contributions to this repo, I was really surprised by the traction it received by many users of your library! The DU-ish functionality seems to be in high demand.

Do you have plans on adding the z.switch() functionality in an upcoming release? I’d love to give it a shot if we know this is the path forward - it doesn’t seem too hard which is a good indication that it’s a good solution 😋.

@colinhacks
Copy link
Owner

colinhacks commented Mar 22, 2024

Do you have plans on adding the z.switch() functionality

@maxArturo

Switch would land in Zod 4, if at all. I'm doing a long-overdue house cleaning right now before starting on Zod 4. I have the advantage of working full-time on Zod for the next couple months. 🤗

At the moment though, I'm leaning toward consolidating everything into z.union(). When a ZodUnion instance is created, it can iterate over its elements and find some fast-and-easy "qualification checks" for each element. When parsing is happening, ZodUnion can run these fast checks for each union element to quickly disqualify certain options before running a full parse.

Put more generally, ZodUnion can get a lot fancier in terms of runtime optimization, making z.discriminatedUnion irrelevant. Ultimately, it will be up to the user to craft a ZodUnion such that the inferred type is a proper discriminated union.


One of the arguments against ZodSwitch is that it's elements cannot be enumerated, since there's no way for Zod to traverse every code path in the switcher function. This would make ZodSwitch a bit of a headache for every ecosystem tool that attempt to do codegen based on Zod schemas.

@sladiri
Copy link

sladiri commented Mar 22, 2024

Just my 2c as a new user of Zod: I found nested discriminated unions intriguing, allows me to have domain logic in types: For example a CRUD form object with or without an id property (create/update), but then nested are the "actual" discriminated unions, certain properties depending on the object kind.

I have used the forks https://github.com/teunmooij/zod-discriminated-union and https://github.com/parisholley/zod-discriminated-union and it nesting is one major feature I see actually.

@andersk
Copy link
Contributor

andersk commented Dec 18, 2024

On top of that, this can be achieved quite simply with .options and destructuring. I've just documented this pattern in the README.

const A = z.discriminatedUnion([ ... ]);
const B = z.discriminatedUnion([ ...A.options, ...more options])

That pattern does not work. TypeScript accepts it at compile time, but Zod rejects it at runtime with Error: Discriminator property <B discriminator> has duplicate value <B discriminator for A>.

import { z } from "zod";

const stream_event_schema = z.discriminatedUnion("op", [
  z.object({ type: z.literal("stream"), op: z.literal("create") }),
  z.object({ type: z.literal("stream"), op: z.literal("update") }),
  z.object({ type: z.literal("stream"), op: z.literal("delete") }),
]);

const event_schema = z.discriminatedUnion("type", [
  z.object({ type: z.literal("message") }),
  z.object({ type: z.literal("presence") }),
  ...stream_event_schema.options,
]);
file:///tmp/test/node_modules/zod/lib/index.mjs:3044
                    throw new Error(`Discriminator property ${String(discriminator)} has duplicate value ${String(value)}`);
                          ^

Error: Discriminator property type has duplicate value stream
    at Object.create [as discriminatedUnion] (file:///tmp/test/node_modules/zod/lib/index.mjs:3044:27)
    at file:///tmp/test/test.ts:9:24
    at ModuleJob.run (node:internal/modules/esm/module_job:268:25)
    at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:543:26)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:116:5)

Moreover, even if it did work, it would lose all information about the inner discriminator, negating all the optimization and diagnostic advantages of the inner discriminatedUnion.

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.

discriminated union cant be nested Allow more types in discriminated unions