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

Add Zod as an optional peer dependency of Astro #11576

Closed
wants to merge 4 commits into from

Conversation

bholmesdev
Copy link
Contributor

Changes

We've found a number of users reach for a manual installation of Zod in their Astro project. This may be to consolidate content validators in a separate package, or to reach for zod-compatable libraries like react-hook-form.

Adding zod as an optional peer dependency should ensure the user's version of zod is at least as recent as the version Astro uses internally. This lowers the risk of incompatibility between user code and Astro code that relies on Zod, like Content Collections or Actions.

Note: Since we use the carat ^ on our peer dependency, the user may still install a more recent version of Zod than Astro uses internally. This can cause compatibility issues. The only solution to this problem is making zod a required peer dependency of Astro. Given this would be required for every Astro project, it feels a step too far.

Testing

Manually tested installation with and without the optional peer dependency. Adding the peer dependency adds one point of security: a warning message when the user's installation of zod is an older version than what Astro expects.

Docs

N/A

Copy link

changeset-bot bot commented Jul 30, 2024

🦋 Changeset detected

Latest commit: c1e6765

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Jul 30, 2024
@github-actions github-actions bot added the semver: major Change triggers a `major` release label Jul 30, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR is blocked because it contains a major changeset. A reviewer will merge this at the next release if approved.

@bluwy
Copy link
Member

bluwy commented Jul 31, 2024

Adding zod as an optional peer dependency should ensure the user's version of zod is at least as recent as the version Astro uses internally. This lowers the risk of incompatibility between user code and Astro code that relies on Zod, like Content Collections or Actions.

Shouldn't this be handled on the package manager level? At least pnpm has pnpm dedupe if you want to ensure a single version of a package is used. Or if you run npm update zod, it'll update all zod versions transitively.

Maybe I'm missing the issues it causes, it would be nice to have reference/links to them.

@bholmesdev
Copy link
Contributor Author

@bluwy perhaps it should! But given the user issues we've heard, it sounds like package managers do not handle this by default. Here is the user story I want to improve:

  1. User runs npm i zod
  2. Later, user runs npm i astro@latest
  3. Their version of zod is older than our version, and types break.

By adding an optional peer dep, the user should be warned at (2) that their version of zod is a mismatched peer dep. Without this, npm doesn't care.

Does that make sense? I understand commands like npm update could smooth over that installation hurdle, but not many would know do use that tool (nor do we document it)

@bluwy
Copy link
Member

bluwy commented Jul 31, 2024

By adding an optional peer dep, the user should be warned at (2) that their version of zod is a mismatched peer dep. Without this, npm doesn't care.

Do you mean that if we bump our zod version, but theirs are still lower, npm will warn of the mismatch? Doesn't us bumping the zod version a breaking change then? As it requires another user action on their end. Before, it's optional if the types don't conflict.

Also, I tested in a project with:

npm install [email protected]
npm install @astrojs/mdx@2 # requires astro ^4.0.0
npm install @astrojs/mdx@latest # requires astro ^4.8.0

I don't see an error/warning from npm, and astro is still 4.7.0 in node_modules.

We also have many types, like shiki or unified exposed to the config that could cause type mismatches, and I think it'd be overkill to expose them all as peer dependencies. I think we should better document using package managers to align the versions in this case.

@bholmesdev
Copy link
Contributor Author

@bluwy You have a great point considering shiki and unified. My only argument would be that a manual Zod installation is more common than a manual shiki installation, so it could deserve extra attention.

I don't see an error/warning from npm, and astro is still 4.7.0 in node_modules.

I'll admit I can't get the warning with npm either; only with pnpm. It is an improvement to be fair, but it's a shame that plain npm doesn't see a benefit.

Overall, I'm okay to close this PR. I'm curious what the best action item is to avoid user issues though? I see a few avenues:

  • Improve guidance from support patrol.
  • Add a feature to @astrojs/upgrade to check for a Zod version, since it's a commonly used package.
  • Add some sort of note to the docs. I'm least in-favor of this, since it's unlikely we'll find a place that catches users at the right time 🤷

Curious if you have any thoughts on this @bluwy

@bluwy
Copy link
Member

bluwy commented Aug 5, 2024

Having zod upgrading together in @astrojs/upgrade would be nice, maybe something we can do together with #9268

For docs, I agree that it's probably hard to find, but maybe a small section at https://docs.astro.build/en/guides/troubleshooting/ regarding types issues and upgrading zod to fix it might help. So something support patrol could also link to.

I'd prefer if we can avoid maintaining an additional peer dep in astro, but thanks for investigating and fixing this issue!

@Princesseuh Princesseuh removed the semver: major Change triggers a `major` release label Aug 14, 2024
@bholmesdev
Copy link
Contributor Author

Sounds good! Made a TODO to add a "common gotchas" entry for this. Thanks @bluwy 👍

@bholmesdev bholmesdev closed this Aug 21, 2024
@bluwy bluwy deleted the feat/action-z-default branch October 8, 2024 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants