-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Upgrade to Vite 4 #5685
Upgrade to Vite 4 #5685
Conversation
🦋 Changeset detectedLatest commit: f6e9a1f 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 |
@@ -139,9 +139,13 @@ export async function loadFixture(inlineConfig) { | |||
let devServer; | |||
|
|||
return { | |||
build: (opts = {}) => build(settings, { logging, telemetry, ...opts }), | |||
build: (opts = {}) => { | |||
process.env.NODE_ENV = 'production'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vite 4 doesn't mutate NODE_ENV
anymore, unless it's undefined
. So if we run the dev server, and then build, build would run in development
NODE_ENV
instead, which broke the MDX test.
This shouldn't affect users in practice, but internally for our tests we need to make sure NODE_ENV
is the right value we're testing.
preprocess: [ | ||
preprocess({ | ||
less: true, | ||
postcss: postcssConfig, | ||
sass: { renderSync: true }, | ||
scss: { renderSync: true }, | ||
stylus: true, | ||
typescript: true, | ||
}), | ||
], | ||
preprocess: [vitePreprocess()], | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vitePreprocess
does the same thing as svelte-preprocess
, except it's relying on Vite's internal processing like we do, so it should be slightly more performant.
This introduced a small breaking change where lang="postcss"
is required for vitePreprocess
to process to minimize preprocess overhead, noted in the changesets.
@@ -65,7 +50,7 @@ function getViteConfiguration({ | |||
|
|||
return { | |||
optimizeDeps: { | |||
include: ['@astrojs/svelte/client.js', 'svelte', 'svelte/internal'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vite-plugin-svelte includes the removed identifiers by default.
addRenderer(getRenderer()); | ||
updateConfig({ | ||
vite: getViteConfiguration({ | ||
options, | ||
isDev: command === 'dev', | ||
postcssConfig: config.style.postcss, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This removal is safe because internally Astro passes it down to Vite, which when we use vitePreprocess
, it would include this configuration.
Probably! I think I remember you saying this is mostly non-breaking? Do we have a quick summary of what Astro users are most likely to run into? Or should everyone read the Vite migration doc? |
I think we might want users to skim through https://vitejs.dev/guide/migration.html first as it's not a lot. But I'd say the most likely breaking change someone might hit is incompatible Vite/Rollup plugins (Rollup 3). The changeset notes could probably be enough, and I bet writing good ones there would ease writing the migration guide later. So perhaps I do need docs maintainers to review the changeset 🤔 PS I updated some tests but I'm expecting the smoke tests to fail still. Will need to dig that deeper. |
Marking as ready for review. Smoke test should still fail as it's blocked by #5706 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read through the Vite 4 migration guide and I think @bluwy’s right we don’t necessarily need to call out too much in terms of Astro migration for this one. So no more docs than the changesets needed for now I think.
Changes
Upgrade to Vite 4, along with Vite plugins that has peer on Vite 4.
Also updated the Svelte integration relying on
vite-plugin-svelte
v2 features, which should simplify it's configuration.Testing
Updated some test utils to work in Vite 4. Existing tests should pass.
Docs
I suppose we would document the migration guide once we're near a stable release? cc @withastro/maintainers-docs