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

chore (client): pin Zod to 3.21.1 to avoid code generator bug in client #700

Closed
wants to merge 4 commits into from

Conversation

fooware
Copy link
Contributor

@fooware fooware commented Nov 27, 2023

Summary

As discussed on Discord with @kevin-dp, it appears that the dependencies for Zod can pull in a version that causes the generated client to contain invalid casts.

image

The issue in Zod + Prisma can be found here:
colinhacks/zod#2184
chrishoermann/zod-prisma-types#98

Background

For us, the problem manifests itself by adding any kind of basic table relationship and generating a client. Indeed, it appears to also happen in this example code created by a third party:
https://github.com/KyleAMathews/vite-react-router-electric-sql-starter/tree/main

In both that example, and our internal code, it is enough to comment out any relationships and the generated code is fine, or rather doesn't contain the code blocks that are causing the issues.

image

Patching the packages lock file in both cases to resolve Zod to 3.21.1 (as suggested in the linked issue threads) fixed the issue for both our code and the third-party example.

image
image

Temporary Workaround

Since we are using PNPM we could achieve the same result by adding a forced version override to our root package.json file:

  "pnpm": {
    "overrides": {
      "zod": "3.21.1"
    }
  }

Using that we could also experiment with different Zod versions, and indeed, going to 3.21.2 or higher reintroduced the problem.

Suggested Fix

This PR pins the version of Zod to 3.21.1 which is the latest version that did not cause the issue. Hopefully, this will mean that new users will get a local package lock file that resolves Zod to 3.21.1 and a working client generator. That last part is a bit hard for me to verify locally though.

Caveat

I have only updated the TypeScript Client and the Client Generator, but there are at least two places in this mono repo that explicitly reference higher versions of Zod:

  • e2e/satellite_client - ^3.21.4
  • examples/linearlite - ^3.22.2

I was not comfortable downgrading those dependencies since I do not know why they require a higher version.

Copy link
Contributor

@kevin-dp kevin-dp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @fooware, thanks for your contribution!
The e2e tests and linearlite should indeed also be pinned to version 3.21.1 of Zod.
The fact that they are using a higher version of Zod is an accident.

Could you also generate a changeset for this PR please?
To do that, run pnpm changeset at the root of the monorepo.

@kevin-dp kevin-dp changed the title Pin Zod to 3.21.1 to avoid code generator bug in client chore (client): pin Zod to 3.21.1 to avoid code generator bug in client Nov 28, 2023
@fooware
Copy link
Contributor Author

fooware commented Nov 28, 2023

Hi @fooware, thanks for your contribution! The e2e tests and linearlite should indeed also be pinned to version 3.21.1 of Zod. The fact that they are using a higher version of Zod is an accident.

Done.

Could you also generate a changeset for this PR please? To do that, run pnpm changeset at the root of the monorepo.

Done, but please make sure I did it correctly.

Please note that I didn't rebuild any packages here. I assume that a build server handles that during release?

@@ -0,0 +1,8 @@
---
"electric-sql": patch
"@core/electric": patch
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changeset is not correct. It patches all packages but your PR only touches the typescript client (i.e. electric-sql) and the generator (i.e. @electric-sql/prisma-generator). It does not touch Electric (@core/electric) neither does it touch the starter ("create-electric-app": patch).

So the changeset should look like this:

---
electric-sql": patch
@electric-sql/prisma-generator": patch
---

Pin Zod to version 3.21.1

You probably accidentally selected all packages instead of only the changed packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assumed it only listed the ones it detected changes in so I accepted the defaults.

I’ll push an updated one tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, since you changed the ts-client and the generator it should ask something like:

Which packages would you like to include? … 
◯ changed packages
  ◯ electric-sql
  ◯ @electric-sql/prisma-generator
◯ unchanged packages
  ◯ @core/electric
  ◯ create-electric-app

And you should only check "changed packages" (by pressing the spacebar).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevin-dp : It didn't show it like that for me, but perhaps that is me not branching in some expected way.
image

I have now re-generated the changeset as per your instructions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevin-dp, any feedback? I assume I shouldn’t resolve this thread?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @fooware, sorry for the delay. We are discussing this internally to see whether we want to pin Zod to a specific version (which affects all user projects) or if we can provide a less invasive fix. Will keep you posted today hopefully!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevin-dp, any feedback? I assume I shouldn’t resolve this thread?

We will need a bit more time to see if we can provide a fix for this under the hood such that we don't need to pin the version of Zod to a specific version for all Electric projects. This will require some changes in the generator. I'm hoping to look into it tomorrow and ideally push a fix for it tomorrow. We will keep you posted here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The zod issue is open for some time now and has still not been confirmed, so I think there will not be a fix any time soon. I circumvented the issue by adding an option to use type assertions in zod-prisma-types. It is a brute force method so I can personally use it with latest zod versions and it bypasses all the nice type inference, but since the library is about runtime checks - and these seem to work - it is an acceptable risk I'm willing to take imost of the time

I don't know if this is a valid option in your case but since the types worked pre 3.21.2 there seems to be nothing wrong with the prisma types and with asserting them.

samwillis pushed a commit that referenced this pull request Dec 11, 2023
This PR fixes the problem where the generated client may contain type
errors for models containing relations.
This is a known problem with Zod and Prisma and is described here:
chrishoermann/zod-prisma-types#98. That issue
also proposes a solution with a dirty type cast:
chrishoermann/zod-prisma-types#98 (comment)
which is implemented in this PR.

This PR removes the need for pinning the version of Zod
(#700).
@alco
Copy link
Member

alco commented Dec 12, 2023

@kevin-dp @samwillis does #727 address this issue?

@samwillis
Copy link
Contributor

Hey @fooware,

Thanks for the help with this, we fixed the issue in #727 with your tip off for the upstream bug report. We will be publishing a version with this fixed this week.

@samwillis samwillis closed this Dec 12, 2023
kevin-dp added a commit that referenced this pull request Apr 30, 2024
…1187)

Every Electric app needs Zod in order to use the generated client.
However, in recent versions of Zod something changed that breaks the
types in the generated client (cf.
#700). Therefore, Electric
apps should always use Zod version 3.21.1. Added this as a peer
dependency such that npm complains if the app installs a different
version. Similarly, added an optional peer dependency for Prisma v4 such
that it complains if the app installs another version of Prisma.
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.

5 participants