fix: multiple entries with shared css and no JS

[patak-dev]

Fixes #13436

Edited Description

rollup started to share entry points chunks (I don't have an exact reference on when this happened). For the added test case, if you run it in Vite 3, you'll see the following chunks:

shared-css-no-js-{}.js (modules: shared-css-no-js.html)
shared-css-with-js-{}.js (modules: shared-css-with-js.html, src/shared-css-main.js)
shared-css-theme.js (modules: shared-css-theme.css)

With the first two chunks importing shared-css-theme.js

In Vite 4.x, the test would result in:

shared-css-no-js-{}.js (modules: shared-css-no-js.html, shared-css-theme.css)
shared-css-with-js-{}.js (modules: shared-css-with-js.html, src/shared-css-main.js)

With the second chunk importing the first one.

This broke current assumptions in the CSS and HTML Plugin.

The fix this PR applies is to use moduleSideEffects: 'no-treeshake' when generating the JS entry points corresponding to each HTML entry point. This reverts to the previous structure where the entry points aren't shared. The empty shared-css-no-js-{}.js chunk is correctly removed as before by the HTML plugin generateBundle

• See failing test before the fix

Previous Description

The issue is that the CSS chunk now contains a reference in chunk.modules to an HTML file, so it ends up being marked with isPureCssChunk = false because the .html isn't in styles. Testing with a prev version of Vite (and so a prev version of Rollup) the HTML file isn't in modules. It may be because of what is explained in #13608, that entry chunks are now shared. See related change here.

I don't know if this is something we should report to Rollup, but there were references to HTML files in chunk modules before. This PR fixes the issue by checking if there is code in the module.

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[sun0day]

Ah #13608 missed the test cases, maybe we could add the test cases to the playground/html, there are already multiple html entries in it.

[patak-dev]

What test cases do you have in mind?

[sun0day]

I mean we could move this PR's test cases to playground/html, no need to create an extra playground/css-nojs.

[patak-dev]

I discovered other issues. @sun0day, do you have a reference for your comment on #13608?

since rollup@3.25.1 would share entry chunks

This may be causing issues and we may need to prevent sharing entry chunks corresponding to HTML files.

[sun0day]

Yeah, I found rollup@3.25.1 sharing entry chunks when I debugged code in https://github.com/vitejs/vite/pull/13608/files#diff-89bae1df62862bb7f4a03d82a1e9cbf4ac6d0c042f21fbbacb0a2238bd050042L828-L840

That's why I delayed removing inline entry chunks in the bundle after checking all htmls.

Well that's weird, I remembered that I had tested the reproduction of #13436 after merging #13608 in my mac, it worked.

[patak-dev]

See edited description at #13962 (comment)

[patak-dev]

/ecosystem-ci run

[vite-ecosystem-ci]

📝 Ran ecosystem CI: Open

suite	result
analogjs	❌ failure
astro	✅ success
histoire	❌ failure
iles	❌ failure
ladle	✅ success
laravel	✅ success
marko	✅ success
nuxt	✅ success
nx	❌ failure
previewjs	✅ success
qwik	✅ success
rakkas	✅ success
sveltekit	✅ success
unocss	✅ success
vite-plugin-pwa	✅ success
vite-plugin-ssr	✅ success
vite-plugin-react	✅ success
vite-plugin-react-pages	❌ failure
vite-plugin-react-swc	✅ success
vite-plugin-svelte	✅ success
vite-plugin-vue	✅ success
vite-setup-catalogue	✅ success
vitepress	✅ success
vitest	✅ success

[patak-dev]

Same projects are currently failing in main.

[patak-dev]

@lukastaegert just in case, our understanding is that Rollup started sharing entry points (see before and after chunks structure in the description). Is there an option we are missing to revert to the previous behavior? If not, using moduleSideEffects: 'no-treeshake' on these entry points seems to be doing the trick and it shouldn't affect tree-shaking in general as these JS corresponding to the HTML entry points only have imports to other modules in the user source code and the JS inlined in the HTML.

[lukastaegert]

What is likely happening is that we had an issue with unnecessary empty chunks being created in situations where an entry chunk only contained re-exports. There was no easy way to fix this, so we added a post processing step that looks for suitable merge targets for such chunks. you should be able to solve this by setting output.experimentalMinChunkSize to 0. This would, however, prevent users from using this option, or it would fail if users explicitly use this option. no-treeshake would also fail in that case where a user chooses a larger value for that option. A long-term solution would be to add a fake side effect like a call to a global identifier that you could remove again in renderChunk. Then the chunk would only be merged with another entry if the other entry really does import this chunk as otherwise Rollup would create side effect pollution, which the algorithm is designed to prevent.

[patak-dev]

Thanks for the context and explanation!

no-treeshake would also fail in that case where a user chooses a larger value for that option.

Do you have an example of how this could happen? We're returning moduleSideEffects: 'no-treeshake' on the transform hook for these entry points here. The user would need to return a different moduleSideEffects for this module in another transform hook for this to be an issue, no?

A long-term solution would be to add a fake side effect like a call to a global identifier that you could remove again in renderChunk. Then the chunk would only be merged with another entry if the other entry really does import this chunk as otherwise Rollup would create side effect pollution, which the algorithm is designed to prevent.

I thought about this option too. I was using globalThis.__vite_entry(); and indeed fixes the problem. But using no-treeshake on entries seems cleaner to me so users don't see these in the code (that they could also potentially mess up with? I think it is safe though). I'm leaning towards merging this PR and if there are reports of issues with the approach we can migrate to injecting global identifier.

[lukastaegert]

The user would need to return a different moduleSideEffects for this module in another transform hook for this to be an issue, no?

No, I mean even with this option, but I am not at a computer and cannot check, probably I am wrong. If you have a test case that fails without your fix, you should check if your fix still works if you set output.experimentalMinChunkSize to a high value like 10000.

[patak-dev]

Ah, I see. I tried it out using experimentalMinChunkSize: 10000 and it seems it still works correctly. I see logged:

Initially, there are
12 chunks, of which
12 are below minChunkSize.
After merging chunks, there are
12 chunks, of which
12 are below minChunkSize.

Let's merge this then and we now know we could later migrate to injecting a global if we find issues with this approach. Thanks again Lukas!