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

Resolve peer dependency problem in the integration packages (SolidJS, Vue, Svelte, React) #12481

Merged
merged 9 commits into from
Nov 21, 2024

Conversation

marbrex
Copy link
Contributor

@marbrex marbrex commented Nov 20, 2024

Changes

Moved vite from devDependencies to the dependencies of @astrojs/solid-js in its package.json.

Before

Yarn in PnP mode produces the following error :

failed to import "@astrojs/solid-js"
|- Error: vite-plugin-solid tried to access vite (a peer dependency) but it isn't provided by your application; this makes the require call ambiguous and unsound.

Required package: vite (via "vite/package.json")
Required by: vite-plugin-solid

After

The problem is resolved. Astro works fine.
Closes #12460.

Description

This PR addresses an issue (#12460) where @astrojs/solid-js does not declare vite as a dependency in its package.json. As a result, users encounter a peer dependency error when using this integration, particularly because vite-plugin-solid relies on vite but cannot find it unless explicitly provided by the consuming project.

While astro itself includes vite as a dependency, peer dependencies cannot rely on transitive dependencies. This causes issues when configuring the SolidJS integration in the astro.config.js, as @astrojs/solid-js is unable to resolve vite.

By adding vite as a dependency of @astrojs/solid-js, this PR ensures :

  1. The peer dependency for vite in vite-plugin-solid is resolved properly.
  2. Users no longer need to manually add vite to their project's dependencies or use workarounds like Yarn's packageExtensions.

Testing

Tested locally with Yarn's packageExtenstions.

Docs

Not sure if docs should be updated or not.
/cc @withastro/maintainers-docs for feedback!

Copy link

changeset-bot bot commented Nov 20, 2024

🦋 Changeset detected

Latest commit: d8a7f5d

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 pkg: solid Related to Solid (scope) pkg: integration Related to any renderer integration (scope) labels Nov 20, 2024
@ematipico
Copy link
Member

What if we moved vite inside the peerDependencies? It feels like a better fix to me 🤔

@marbrex
Copy link
Contributor Author

marbrex commented Nov 20, 2024

@ematipico It could be another solution, but in this case it would mean that users will still need to manually install vite in their project to satisfy the peer dependency, which can lead to confusion if they aren't familiar with managing peer dependencies. This could reintroduce the original issue.

@ematipico
Copy link
Member

but in this case it would mean that users will still need to manually install vite in their project to satisfy the peer dependency

In this case, no, because Astro installs vite as a direct dependency, so the users wouldn't need to do anything. Plus, other integrations that rely on vite plugins such as @astrojs/svelte and @astrojs/vue don't do anything fancy around vite.

I believe we are hitting a specific issue of Yarn 🤔

@marbrex
Copy link
Contributor Author

marbrex commented Nov 20, 2024

@ematipico Indeed, it seems to be related to the the way Yarn PnP operates. It is possible that the vite instance inside astro's dependency tree is scoped to astro and cannot satisfy a peer dependency declared by another package, @astrojs/solid-js in this case.

@bluwy
Copy link
Member

bluwy commented Nov 20, 2024

Yeah if it's a peer dep, the user has to install vite themselves. It can't access Astro's vite dep.

This issue is a little tricky because it's not only affecting solid. svelte, vue, etc are also affected. It worked with pnpm because it hoists packages by default (more like private hoisting than a public one, which is what npm is doing).

I guess the right thing to do given what we have is to put vite in dependency like in this PR, but I think we should do this for solid, svelte, vue, react, preact all at once.

@marbrex
Copy link
Contributor Author

marbrex commented Nov 20, 2024

@bluwy Yes, I think so as well. I'm happy to adjust the PR accordingly! 😊

@github-actions github-actions bot added pkg: svelte Related to Svelte (scope) pkg: vue Related to Vue (scope) pkg: react Related to React (scope) labels Nov 20, 2024
@marbrex marbrex changed the title Resolve peer dependency problem in the SolidJS integration package Resolve peer dependency problem in the integration packages (SolidJS, Vue, Svelte, React) Nov 20, 2024
@github-actions github-actions bot added the pkg: preact Related to Preact (scope) label Nov 20, 2024
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

I think the only concern with this is potential Vite version mismatches, but hopefully the package manager is smart enough to dedupe it.

Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
@marbrex
Copy link
Contributor Author

marbrex commented Nov 21, 2024

@bluwy @ematipico Seems like one of the test jobs timed out. The time has exceeded 25 minutes.

@ematipico ematipico merged commit 8a46e80 into withastro:main Nov 21, 2024
14 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope) pkg: preact Related to Preact (scope) pkg: react Related to React (scope) pkg: solid Related to Solid (scope) pkg: svelte Related to Svelte (scope) pkg: vue Related to Vue (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integration "@astrojs/solid-js" does not provide "vite" to "vite-plugin-solid"
3 participants