zod icon indicating copy to clipboard operation
zod copied to clipboard

#1171 support for refine, superRefine, transform and lazy in discriminatedUnion

Open roblabat opened this issue 3 years ago • 2 comments

Fix #1171

Also add support for lazy, refine and superRefine in discriminatedUnions

roblabat avatar Jul 26 '22 15:07 roblabat

Deploy Preview for guileless-rolypoly-866f8a ready!

Name Link
Latest commit c39d1746f179d39290867566953bf76d54d83303
Latest deploy log https://app.netlify.com/sites/guileless-rolypoly-866f8a/deploys/63734fd48b336a00080d614f
Deploy Preview https://deploy-preview-1290--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.

netlify[bot] avatar Jul 26 '22 15:07 netlify[bot]

@roblabat Looking good! I'll try to take some time in the next day or two to look a little more deeply at the implementation here, but first quick pass looks promising, thanks for taking this on.

scotttrinh avatar Aug 17 '22 14:08 scotttrinh

Just ran into this problem, anything holding up this PR?

benlongo avatar Nov 02 '22 17:11 benlongo

Unfortunately I don't think getting this clever with extraction of literal values is wise. I modified this PR to make z.discriminatedUnion moderately more robust, but all union options still have to be ZodObject instances. Technically your approach that unwraps ZodEffects isn't sound, since ZodEffect is the class that contains preprocess/transform logic. The discriminator key must be available on the input at the outset in order to determine which option to use. It must also exist in the Output type of the schema, otherwise the discriminated union has been broken. Supporting ZodEffect instances jeopardizes both of these and increases the odds someone shoots themselves in the foot here.

I dislike a lot of things about z.discriminatedUnion, but mostly that it's implementation depends on the internals of a bunch of first-party Zod types. In theory, anyone could subclass ZodType to create their own Zod schema classes, but using all this internal logic breaks portability. Approaches like the one in this PR make that even more true, which is why I'm slightly allergic to it.

If you want complex unions with refine, transforms, lazy, etc, I'd recommend just using z.union. ZodDiscriminatedUnion is intended as an optimization for simple cases.

colinhacks avatar Nov 15 '22 08:11 colinhacks

Unfortunately I don't think getting this clever with extraction of literal values is wise. I modified this PR to make z.discriminatedUnion moderately more robust, but all union options still have to be ZodObject instances. Technically your approach that unwraps ZodEffects isn't sound, since ZodEffect is the class that contains preprocess/transform logic. The discriminator key must be available on the input at the outset in order to determine which option to use. It must also exist in the Output type of the schema, otherwise the discriminated union has been broken. Supporting ZodEffect instances jeopardizes both of these and increases the odds someone shoots themselves in the foot here.

I dislike a lot of things about z.discriminatedUnion, but mostly that it's implementation depends on the internals of a bunch of first-party Zod types. In theory, anyone could subclass ZodType to create their own Zod schema classes, but using all this internal logic breaks portability. Approaches like the one in this PR make that even more true, which is why I'm slightly allergic to it.

If you want complex unions with refine, transforms, lazy, etc, I'd recommend just using z.union. ZodDiscriminatedUnion is intended as an optimization for simple cases.

@colinhacks I don't understand why the discriminator key should be available in the output of the discriminated union. DiscriminatedUnion is just an optimisation of unions with this discriminator key helping the code to find the correct type apply quicker. But like in a union the output doesn't have to be one inputs but one of the outputs of all types of the union if no output type of the discriminated union have the discriminator key I don't see why it should in the output has it already made it's office selecting the correct type of the union.

By the way I'm not really happy with the implementation like you as it's much more a work around to make it work with ZodEffect and ZodLazy where it should be working by checking the key of the Input of the ZodType to really make it's office. But I currently don't have a solution to do that properly.

But To conclude for me the PR should be reopened as your commit revert all the implementation of the PR and related issues or no more solved by this merge and should then be reopened as this merge doesn't solve issues asking for a discriminatedUnion working with transform, refine, superRefine and lazy.

ps: Even if using unions could work I think discriminatedUnion is a good optimisation pattern that should be more than just a quick a dirty solution working on just objects. It can be really powerfull especially with transform.

roblabat avatar Nov 15 '22 13:11 roblabat

Release notes say "There are no breaking API changes", but there are at least 2 breaking changes in this pull request:

  1. The signature of ZodDiscriminatedUnion was changed from class ZodDiscriminatedUnion< Discriminator extends string, DiscriminatorValue extends Primitive, Option extends ZodDiscriminatedUnionOption<Discriminator, DiscriminatorValue>> to class ZodDiscriminatedUnion< Discriminator extends string, Options extends ZodDiscriminatedUnionOption<Discriminator>[]>

  2. options property of ZodDiscriminatedUnion was renamed to optionsMap and new options property introduced, having different type.

Naktibalda avatar Dec 13 '22 08:12 Naktibalda