withastro / astro

The web framework for content-driven websites. ⭐️ Star to support our work!
https://astro.build
Other
46.93k stars 2.49k forks source link

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

Closed marbrex closed 1 day ago

marbrex commented 2 days ago

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!

changeset-bot[bot] commented 2 days ago

🦋 Changeset detected

Latest commit: d8a7f5de046a379b17aadcdc3a4671269cb2ca64

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

ematipico commented 2 days ago

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

marbrex commented 2 days ago

@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 commented 2 days ago

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 commented 2 days ago

@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 commented 1 day ago

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 commented 1 day ago

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

marbrex commented 1 day ago

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