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

Update zod dependency to latest (v3.22.4) #739

Merged
merged 2 commits into from
Oct 11, 2023
Merged

Conversation

Lordfirespeed
Copy link
Contributor

@Lordfirespeed Lordfirespeed commented Oct 5, 2023

Of note, Zod < 3.22.3 is vulnerable to ReDOS.
Would recommend updating the dependency ASAP.

Zod's changelog doesn't obviously reflect any breaking API changes from 3.21.1 -> 3.22.4, and I'm pretty sure Zod adheres to SemVer so the version numbers agree with this.

@Lordfirespeed
Copy link
Contributor Author

package-lock.json needs to be rebuilt via npm install, but I don't have a development environment checked out

@ymc9
Copy link
Member

ymc9 commented Oct 6, 2023

Thanks! Right now the dependency to zod is only used in the CLI but not runtime, but still good to upgrade to a newer version. I'll fix the lock file and then merge.

@Lordfirespeed
Copy link
Contributor Author

Are you sure?

It looks to me like the generated Zod schemas plugin would mean Zod is a runtime dependency (if that plugin is enabled) - is there something I'm missing?

@ymc9
Copy link
Member

ymc9 commented Oct 7, 2023

Are you sure?

It looks to me like the generated Zod schemas plugin would mean Zod is a runtime dependency (if that plugin is enabled) - is there something I'm missing?

My apologies ...

The "@core/zod" plugin is only design time and handles Zod schema code generation. It assumes that the developer's project has a compatible version of Zod installed - to compile the generated code and run it.

However, the "@zenstackhq/runtime" package today does have a dependency on "zod" package as well. That's for evaluating field-level validation rules like:

model User {
    ...
    email String @email
}

My thought is that we should turn that dependency into a peer dependency but not force a specific version of Zod. What do you think @Lordfirespeed ?

@Lordfirespeed
Copy link
Contributor Author

In my experience, peer dependencies in NPM are intended for 'plugins', where the 'plugin' package expects some 'host' to be installed, but never actually imports from the host.

An example of this is eslint and any of its plugins for TypeScript; eslint is the host, and it imports the code from the plugins - but since people can choose what plugins they want, eslint can't specify a dependency on all the plugins that exist. Hence instead, eslint is specified a peer dependency in all the plugins.

In ZenStack's case, the runtime does import from Zod, and therefore directly depends on it, so Zod should not be a peer dependency.

Perhaps the runtime Zod dependency version should be given with a more general version specifier, like this:

"zod": "^3.22.4",

This way, the runtime specifies that it is happy to work with any version of Zod newer than 3.22.4, until version 4.x (which we can presume would include breaking changes)

Presently there are various runtime dependencies specified in this manner.

I saw that you posted a message about an incompatability with Zod 3.22 in Discord - is that a problem?

@ymc9
Copy link
Member

ymc9 commented Oct 10, 2023

In my experience, peer dependencies in NPM are intended for 'plugins', where the 'plugin' package expects some 'host' to be installed, but never actually imports from the host.

An example of this is eslint and any of its plugins for TypeScript; eslint is the host, and it imports the code from the plugins - but since people can choose what plugins they want, eslint can't specify a dependency on all the plugins that exist. Hence instead, eslint is specified a peer dependency in all the plugins.

In ZenStack's case, the runtime does import from Zod, and therefore directly depends on it, so Zod should not be a peer dependency.

Perhaps the runtime Zod dependency version should be given with a more general version specifier, like this:

"zod": "^3.22.4",

This way, the runtime specifies that it is happy to work with any version of Zod newer than 3.22.4, until version 4.x (which we can presume would include breaking changes)

Presently there are various runtime dependencies specified in this manner.

I saw that you posted a message about an incompatability with Zod 3.22 in Discord - is that a problem?

Sorry for the late response. Been a bit heads down working on documentation 😂.

I think the way ZenStack uses Zod is a bit unclean due to some legacy design issue and later changes. It's more like a plugin. User opts in to zod features, mainly in three ways:

  1. Explicitly declaring the @core/zod plugin

    plugin zod {
        provider = '@core/zod'
    }

    The Zod schemas are generated for models, compiled against the version of Zod that the user installed, and reexported the schemas through @zenstackhq/runtime/zod. The user decides how to use them.

  2. Using trpc plugin, which has a dependency to the @core/zod plugin (for input validation)

  3. Using data validation attributes

    model User {
        ...
        email String @email
    }

    It activates the @core/zod plugin. @zenstackhq/runtime loads the generated schema and enforces the validation. In this case, ZenStack's runtime does directly depend on Zod, but more in an "opt-in" way. Maybe a cleaner solution is add some abstraction to the generated Zod schemas, so runtime can load and parse it with but without a direct dependency to Zod.

I'm still a bit torn about how to treat it, but hope I explained it clearly.

@Lordfirespeed
Copy link
Contributor Author

Lordfirespeed commented Oct 10, 2023

Perhaps what you want is to specify Zod as an optional dependency in the runtime, and then handle errors relating to Zod not being installed by providing a helpful message like: "Zod isn't installed. Install zod with npm install zod@^3".

That would look like this in package.json:

{
  "optionalDependencies": {
    "zod": "^3.22.4"
  }
}

In practise, this means Zod will still be installed by default, but an installation failure relating to Zod will not cause ZenStack's installation to fail.

Running npm install @zenstackhq/runtime --omit=optional would not install Zod (and you could set the CLI init command to use this option)

I stand by what I said, though: Zod is definitely not a peer dependency of the runtime.

@ymc9
Copy link
Member

ymc9 commented Oct 10, 2023

Understood. Zod is also not optional when user opts in 😄.

I'll say let's just keep it as a regular dependency for the runtime package for now. Probably not really hurting. I agree with you that the dependency should've put "^3.22.4".

I suspect all the weird compilation issues we saw previously related zod are due to having multiple versions of it. Using a loose version spec should mitigate it to some extent.

@ymc9
Copy link
Member

ymc9 commented Oct 11, 2023

@Lordfirespeed I'm updating this pr to upgrade both zod and zod-validation-error to latest version. Will merge and make it part of V1.1. Thank you!

@ymc9 ymc9 merged commit 30b95eb into zenstackhq:dev Oct 11, 2023
@ymc9
Copy link
Member

ymc9 commented Oct 11, 2023

Fixes #738 #742

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.

2 participants