-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add plugin for Effect #3445
Add plugin for Effect #3445
Conversation
✅ Deploy Preview for guileless-rolypoly-866f8a ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm with the effect team, hi!
I wasn't sent by Mike, he might pop in unrelated to my review
I wanted to go over the impl and give feedback but the impl is so simple and idiomatic I don't have anything missing to add 😄
We have been asked many times how to integrate zod with effect, and we finally have a great answer.
The inclusion of _tag
is just 💯
This is cool! Made some suggestions here: #3449 |
Some design suggestions: Effect doesn't make a difference between sync and async, it feels more appropriate to just expose something like Regarding how to make an effect which can be either sync or async you could use Effect.async((resume) => {
doStuff(() => {
resume(Effect.succeed/fail)
})
}) while the name says async it would be more appropriate to call it callback given that it doesn't mind if the callback is called in sync or async |
Unfortunately Zod doesn't actually know ahead of time if a schema contains async transforms are not. If it encounters a |
Effect has a similar issue but when you use runSync and an async op is found the Error thrown contains a continuation that the caller can then await asynchronously, wondering if it can be possible here too so that instead of re-doing we continue |
It certainly should be possible for Zod's parsing engine to return a Promise only when necessary. In Zod 4 the parsing engine is getting refactored, so I'll try to include this as a design constraint. I agree that a single
Hm interesting, I'll ponder that. The dumb version of that is for the continuation to just re-run the whole parse with But from a user perspective I think a default-async method + an opt-in "enforce sync" is a pretty good DX. The only reason for an Effect user to enforce synchronous is performance. |
In reality given that Effect doesn't use promises nor forcing next ticks the only reason to force sync is when you absolutely have to, e.g. you're doing something in a react component that should occur before render or similar edge cases. Looking forward to Zod 4 |
@mikearnaldi I'm a little fuzzy on your recommendation here. Do you think this should use the continuation approach is better, or are you happy with the two-method API? If you're ok with two methods...any nitpicks around naming? Deferring to you on this. |
I don't have a string preference but if the continuation approach is inefficient then two methods would be fine. Naming isn't my best quality :) |
I think for the names, you could match the conventions you have already established. // non-effect
schema.parse(...)
schema.parseAsync(...)
// effect
schema.parseEffect(...)
schema.parseAsyncEffect(...) Bit longer, but keeps all the parsing apis in the same place. |
I had a quick look, and it seems zod will always return a Promise in "async mode"? So two seperate apis makes sense. |
That's correct, though liable to change in Zod 4. But this is intended to work with Zod 3, so I'll go with two APIs 👍 |
Dear @colinhacks , zod/plugin/effect/src/index.ts Lines 24 to 26 in 402288a
could you please explain: is altering the prototypes of |
@RobinTail Yes, this is how plugins are going to work. The Vue (v2) and Dayjs plugin systems are the prior art here. Zod has always been hackable - that's why it exports so many helper types and utilities. Extending prototypes is a part of JavaScript like any other, and it's I think it's a very underutilized pattern. I'm not aware of any other way to achieve similar functionality. Zod could provide some helper functions to make this look cleaner (e.g. |
* Update README_ZH.md (colinhacks#3433) fix Demo -> Deno * Clean up code, fix build/test * Write docs * Fix rollup build * Fix setup-deno * Add types field * Fix types * Use globalThis check * Add _tag to ZodError * Comments * Add better tests * suggestions for effect plugin (colinhacks#3449) * Updates * Move to .effect.parse() * Bind this in getter * Clean up --------- Co-authored-by: sdshaoda <21106848+sdshaoda@users.noreply.github.com> Co-authored-by: Tim <hello@timsmart.co>
Yay thanks for making this "officially" okay. I was pretty iffy about it to being a decent approach |
EDIT May 2nd. Per some feedback, the new API for this is as follows:
Admittedly, this is a bit of trial run for a Zod plugin system I plan to announce more formally in Zod 4. But also Effect is such a perfect use case for a Zod plugin that I couldn't resist!
Without a plugin, a Zod + Effect integration would require some kind of adapter/resolver system, and imo those always feel a little hacky. With a plugin, we can add strongly-typed Effect-specific methods to the
ZodType
base class with one line of code.This has already been published to
@zod-plugin/effect@0.0.x
if people want to try it. Sample usage:I'm far from an Effect expert, so I'd appreciate any feedback from @gcanti or @mikearnaldi. Zod was mentioned in passing in the 3.0 launch post - I'm curious what kind of API you had in mind and how this compares. This is intentionally super minimal. I admit I haven't fully wrapped my head around the API surface of
@effect/schema
.API alternatives
Would it be more conventional to rename
.effect()
to.effectPromise()
so there's tighter agreement with Effect's.run*()
methods? I don't love this personally but agreement with convention is more important. Effect isn't consistently explicit with thesync/promise
dichotomy in its APIs (e.g. it's.try
instead of.trySync
) so I thought I might be able to get away with just.effect
. 😅I also briefly considered a
.effect()
method that returns some kind ofZodEffect
instance. This would make it possible to configure any Effect specific stuff in a params object. The result could contain Effect-ified versions of Zod's usual methods:.parse
,.parseAsync
,.safeParse
,.safeParseAsync
:Or if we think no configuration is necessary:
I also wasn't clear if the parse input should be considered a Requirement. There wasn't much on the Creating Effects page about Requirements.
Async effects w/ errors
Small thing: I found myself looking for an easier way to instantiate asynchronous
Effect<A, E>
more easily. Something likesuspend
but that can return a promise (though I assume there's a good reason why this isn't supported). I'm using the.try/.tryPromise
methods but I believe it relies on try/catch internally? That'll be a problem for performance-sensitive stuff like Zod.Here's what I want to be able to do:
Side-effectful plugins
I designed this plugin to work as a simple side-effect import.
I'm not sure how will this will work in conjunction with modern frameworks that lack an obvious single "entrypoint", but I think there's probably a place you could put this in, say, a Remix/Next.js application where it'll always get properly bundled/executed before other code. If anyone has experience with other plugin systems like this, chime in. The alternative is to do a dependency injection thing:
But since Zod is a peer dependency of
@zod-plugin/effect
, this isn't actually necessary afaict.