search

edmundhung / conform

A type-safe form validation library utilizing web fundamentals to progressively enhance HTML Forms with full support for server frameworks like Remix and Next.js.
https://conform.guide
MIT License
2k stars 107 forks source link

Remove usage of `instanceof` for Zod schemas #601

Closed colinhacks closed 6 months ago

colinhacks commented 6 months ago

I got a weird bug report on Twitter here: https://twitter.com/Varkoffs/status/1783118133848400070

It seems to be due to a weirdness in Remix, where both the ESM and CommonJS versions of Zod are loaded and used during server initialization. Somehow a schema declared in a Remix page (using ESM) was getting passed into a parseWithZod call that was using CJS internally to do instanceof checks. Because each copy of Zod has separate class definitions, the instanceof checks fail.

This is by no means an issue with conform, but one possible solution is for conform to avoid instanceof. There are other scenarios where both builds of a package can be loaded simultaneously. Some are described by isaacs (founder of npm) here: https://github.com/isaacs/tshy?tab=readme-ov-file#dual-package-hazards

The tldr is that its best to avoid instanceof when possible, especially for libraries that are referencing classes from their dependencies.

This PR technically relies on "internal" _def APIs but these haven't changed in a long time and you can consider them stable. (Zod 4 will break this, but that's an issue for another day.) Apologies for some of the hackiness/casting but I didn't see a way to avoid it.

Varkoff commented 6 months ago

Thank you for offering a submission.

Here is the issue if you want to try it

Codesandbox https://codesandbox.io/p/github/Varkoff/remix-monorepo-zod-issue/main?import=true Repository https://github.com/Varkoff/remix-monorepo-zod-issue

edmundhung commented 6 months ago

Thanks for making the changes, @colinhacks!

This PR technically relies on "internal" _def APIs but these haven't changed in a long time and you can consider them stable. (Zod 4 will break this, but that's an issue for another day.) Apologies for some of the hackiness/casting but I didn't see a way to avoid it.

I think this is a good workaround. I ran into this issue before but I didn't realize I could get rid of instanceOf completely. The existing logic relies on the internal APIs already so what you did is totally fine. 👍🏼