fix: ZodEffect in discriminatedUnion #2441
Conversation
✅ Deploy Preview for guileless-rolypoly-866f8a ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
| @@ -2882,13 +2882,20 @@ const getDiscriminator = <T extends ZodTypeAny>( | |||
| } | |||
| }; | |||
|
|
|||
| export type ZodDiscriminatedUnionOption<Discriminator extends string> = | |||
There was a problem hiding this comment.
this is a bad diff 😆 . this type is still present on L2895
|
|
||
| export type ZodDiscriminatedUnionOption<Discriminator extends string> = | ||
| | ZodDiscriminatedUnionOptionObject<Discriminator> | ||
| | ZodDiscriminatedUnionOptionEffectsObject<Discriminator>; |
There was a problem hiding this comment.
the creation of this union is the core change in this PR!
| @@ -2985,7 +2992,8 @@ export class ZodDiscriminatedUnion< | |||
|
|
|||
| // try { | |||
| for (const type of options) { | |||
| const discriminatorValues = getDiscriminator(type.shape[discriminator]); | |||
| const shape = "shape" in type ? type.shape : type.sourceType().shape; | |||
There was a problem hiding this comment.
this is shape lookup is the only runtime change! nice n simple
…ffects-in-discriminated-unions
cdaringe
changed the title
|
hey friends, this is a pretty simple PR to review. let me know if there's anything i can do to reduce your review burden. i annotated the changes, for example, as a mechanism to make your review more straight forward. i know maintenance is a huge effort, so I want to be courteous of your precious time! |
|
hey @colinhacks! hope all is well. this PR has been sitting for a bit, and is a pretty staightforward fix. i don't want to irk anyone on the team, but can i expect feedback anytime soon? |
|
Hey all, friendly bump. Thanks for your time and support |
1 similar comment
|
Hey all, friendly bump. Thanks for your time and support |
|
@colinhacks do we have any update on this? Would help us a lot! |
|
I have run into this issue too, would be really helpful if this change could land soon! |
|
I have run into this issue too, would be really helpful if this change could land soon ! 🙏 |
|
This is a well-made PR, but the root problem here is that refinements should be stored inside the schema instead of wrapping the schema with ZodEffects. I'm hesitant to merge because this adds more complexity to a feature (discriminated union) that likely won't exist in Zod 4. The Zod 4 implementation will avoid the fundamental messiness of discriminatedUnion and it's right around the corner, so I'm gonna close. Thanks @cdaringe, this was a great PR. |
|
Almost a year later, checking in? |
Problem
.refineand.superRefined schemas cannot be used indiscriminatedUnion, but should be able to be.See #2440
Discussion
The fix is very simple.
ZodEffects where in the input type is also aZodObject. Add a union member that says "not only accept ZodObject, but ALSO accept ZodEffect<ZodObject>"Add a test. Blamo. Easy!
There's a lot of discussion of "problems with z.discriminatedUnion". I see this as independent from those critiques. This, I perceive, was just a basic oversight in the typing of the original implementation. The fix is simple and carries minimal overhead.