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

feat: expose version at ConfigEnv #19355

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

brillout
Copy link
Contributor

@brillout brillout commented Feb 4, 2025

Description

import { version } from 'vite' isn't reliable: because, with monorepos, it oftens returns the "wrong version" (it returns the version of the locally resolved vite, but not the version of vite that is actually running).

This PR adds ConfigEnv['version'] so that meta frameworks can check whether the user is using a supported Vite version.

For example Vike ensures that the user is using a supported Vite version and shows a helpful error message if the user doesn't:

The only reliable solution I can think of is for Vite to expose its version to Vite plugins.

The earlier the version is exposed the better. For example, exposing the version as ResolvedConfig['version'] would be too late: at that point Vike already uses functionalities of later Vite versions. Exposing the version over ConfigEnv has the advantage that the it's accessible early.

Related PR: #8456.

@brillout brillout mentioned this pull request Feb 4, 2025
@patak-dev
Copy link
Member

Could you provide a reproduction of when import { version } from 'vite' isn't reliable? Wouldn't this cause a tons of others issues?

@brillout
Copy link
Contributor Author

brillout commented Feb 10, 2025

Could you provide a reproduction of when import { version } from 'vite' isn't reliable?

https://github.com/brillout/vike-perma-branches/tree/repro/vite-19355
Minimal reproduction: https://github.com/brillout/repro-vite-19355

Wouldn't this cause a tons of others issues?

Indeed. And that's, for example, one reason why React allows only one React instance.

Vike also does this but less strict: we allow multiple Vike instances but they all must be the same version. Having different versions of Vike within the same process is a no-go and Vike will throw an error and purposely break the app — precisely to avoid a mirad of issues that can arise from this.

That said, for Vite, it's much less of an issue because most Vite plugins don't import that many things from vite. If a Vite plugin doesn't import anything from vite (which is often the case) then there cannot be any issue.

I think whether Vite wants to go strict or loose is something the Vite team should decide. There are trade-offs.

The upside of being strict is that it avoids issues, such as what this PR tries to workaround. The downside is that it's 1) a breaking change and 2) it can be annoying for the user to always have to ensure that the same Vite version is loaded. For example, if a Vite plugin wrongfully adds vite to dependencies instead of peerDependencies then the user will often end up with two different Vite versions and the only way for the user to resolve the issue is by patching the Vite plugin.

I don't know how much being strict would be an annoyance for Vite users, but I'd recommend following course of actions:

  • Merge this PR while labeling ConfigEnv['version'] as experimental so that Vite plugins can start handling this situation.
  • Be strict in the next major Vite release and see how many Vite users complain about it.
  • If too many complain for valid reasons then revert the strictness while keeping ConfigEnv['version'], otherwise keep the strictness and remove ConfigEnv['version'].

@sapphi-red
Copy link
Member

Why do you have vite as both dep and peerDep? IIRC when it's in both dep and peerDep, package managers will treat it as a dep.
https://github.com/vikejs/vike/blob/12968872f8b26633f6389e8cd550fb69ae1e516f/vike/package.json#L144


If we need to expose a version via plugin interface, I'll prefer adding this.meta.viteVersion so that it aligns with rollup.
https://rollupjs.org/plugin-development/#this-meta

@brillout
Copy link
Contributor Author

Why do you have vite as both dep and peerDep? IIRC when it's in both dep and peerDep, package managers will treat it as a dep. https://github.com/vikejs/vike/blob/12968872f8b26633f6389e8cd550fb69ae1e516f/vike/package.json#L144

See vikejs/vike#2070 (comment). It's unrelated to this PR, see minimal reproduction that only depends on vite: https://github.com/brillout/repro-vite-19355.

If we need to expose a version via plugin interface, I'll prefer adding this.meta.viteVersion so that it aligns with rollup. https://rollupjs.org/plugin-development/#this-meta

I just checked and it seems like this is undefined in the config() hook at dev time. But, yea, it would be neat to have a this.meta.viteVersion that is available everywhere.

@tsotimus
Copy link

When developing Vite plugins you eventually need to test your plugin against different vite versions.

@brillout is correct in saying that import { version } from 'vite' is unreliable.

Need this feature badly - happy to find someones already worked on it!

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.

4 participants