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

Upgrade to Vite 4 #5685

Merged
merged 7 commits into from
Jan 3, 2023
Merged

Upgrade to Vite 4 #5685

merged 7 commits into from
Jan 3, 2023

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Dec 28, 2022

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

@changeset-bot
Copy link

changeset-bot bot commented Dec 28, 2022

🦋 Changeset detected

Latest 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

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pkg: svelte Related to Svelte (scope) pkg: vue Related to Vue (scope) pkg: integration Related to any renderer integration (scope) labels Dec 28, 2022
@bluwy bluwy added the semver: major Change triggers a `major` release label Dec 28, 2022
@@ -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';
Copy link
Member Author

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.

Comment on lines -29 to 24
preprocess: [
preprocess({
less: true,
postcss: postcssConfig,
sass: { renderSync: true },
scss: { renderSync: true },
stylus: true,
typescript: true,
}),
],
preprocess: [vitePreprocess()],
};
Copy link
Member Author

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'],
Copy link
Member Author

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,
Copy link
Member Author

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.

@bluwy bluwy marked this pull request as draft December 28, 2022 09:41
@delucis
Copy link
Member

delucis commented Dec 28, 2022

I suppose we would document the migration guide once we're near a stable release? cc @withastro/maintainers-docs

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?

@bluwy
Copy link
Member Author

bluwy commented Dec 28, 2022

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.

@bluwy bluwy marked this pull request as ready for review January 2, 2023 12:21
@bluwy
Copy link
Member Author

bluwy commented Jan 2, 2023

Marking as ready for review. Smoke test should still fail as it's blocked by #5706

Copy link
Member

@delucis delucis left a 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.

@matthewp matthewp merged commit f6cf92b into main Jan 3, 2023
@matthewp matthewp deleted the vite-4 branch January 3, 2023 18:24
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) pkg: integration Related to any renderer integration (scope) pkg: svelte Related to Svelte (scope) pkg: vue Related to Vue (scope) semver: major Change triggers a `major` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants