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

fix(ssr): remove pure CSS dynamic import #17371

Merged

Conversation

sapphi-red
Copy link
Member

Description

The pure chunk was removed but the dynamic import importing that chunk was not removed.
This if condition is true for SSR code.

if (format !== 'es' || ssr || isWorker) {

Dynamic import for non-SSR code is removed here.
const removedPureCssFiles =
removedPureCssFilesCache.get(config)!
const chunk = removedPureCssFiles.get(filename)
if (chunk) {
if (chunk.viteMetadata!.importedCss.size) {
chunk.viteMetadata!.importedCss.forEach((file) => {
deps.add(file)
})
hasRemovedPureCssChunk = true
}
s.update(expStart, expEnd, 'Promise.resolve({})')
}

fixes #17366
refs #16078

@sapphi-red sapphi-red added feat: css p3-minor-bug An edge case that only affects very specific usage (priority) feat: ssr regression The issue only appears after a new release labels Jun 1, 2024
Copy link

stackblitz bot commented Jun 1, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

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.

LGTM. I wished we could abstract the code so we don't have to duplicate it, but it seems a bit tricky.

@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 943221f: Open

suite result latest scheduled
nuxt failure failure
sveltekit failure success
vite-plugin-react-pages failure success
vitest failure failure

analogjs, astro, histoire, ladle, laravel, marko, previewjs, quasar, qwik, rakkas, remix, unocss, vike, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

The vite-plugin-react-pages is a fluke. The SvelteKit one seems to be a snapshot-like issue after this change. Approving as it looks ok to merge this as part of the 5.3 beta. @sapphi-red letting you merge it though in case you want to check out SvelteKit

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 943221f: Open

suite result latest scheduled
histoire success failure
nuxt failure failure
sveltekit failure failure
vite-plugin-react-pages failure failure
vitest failure failure

analogjs, astro, ladle, laravel, marko, previewjs, quasar, qwik, rakkas, remix, unocss, vike, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress

@bluwy
Copy link
Member

bluwy commented Jun 6, 2024

The SvelteKit tests has been failing on various different tests on each run, so it's hard to tell this is causing it to fail. I'll merge this for now and will backport to 5.2.13 soon.

@bluwy bluwy merged commit 67ff94b into vitejs:main Jun 6, 2024
11 checks passed
bluwy pushed a commit that referenced this pull request Jun 7, 2024
@bluwy bluwy mentioned this pull request Jun 7, 2024
@sapphi-red sapphi-red deleted the fix/ssr-remove-pure-css-dynamic-import branch July 6, 2024 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: css feat: ssr p3-minor-bug An edge case that only affects very specific usage (priority) regression The issue only appears after a new release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug Report] 5.2.12 regression - dynamic css import will cause MODULE_NOT_FOUND in SSR build
3 participants