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

Remove usage of instanceof for Zod schemas #601

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

colinhacks
Copy link
Contributor

@colinhacks colinhacks commented Apr 24, 2024

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.

@colinhacks colinhacks changed the title Remove usage of instanceof Remove usage of instanceof for Zod schemas Apr 24, 2024
@Varkoff
Copy link

Varkoff commented Apr 25, 2024

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
Copy link
Owner

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. 👍🏼

@edmundhung edmundhung merged commit b4d4817 into edmundhung:main Apr 25, 2024
2 checks passed
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.

3 participants