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: set scripts imported by HTML moduleSideEffects=true #18411

Conversation

sapphi-red
Copy link
Member

Description

When setting moduleSideEffects: false, all scripts gets tree-shaked and the built output becomes empty. This is because we convert HTML files into a JS file just containing side effect imports that points to the href attr of script tags.

While the behavior is correct (the scripts do have a side-effect and setting it false is a lie) and kind of expected, it is more easier to configure if these scripts are marked as moduleSideEffects: true as in most cases you won't rely on side-effect imports. (The correct way of fixing this is to set treeshake.moduleSideEffects: (id) => scriptsImportedByHtml.includes(id) instead.)

fixes #16720
fixes #17911

This PR reverts #12786 as this fix covers that one as well. (To fix that one correctly, the sideEffects field should include the scripts imported by the HTML file.)

@sapphi-red sapphi-red added the p2-to-be-discussed Enhancement under consideration (priority) label Oct 21, 2024
@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci

This comment was marked as outdated.

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on cb81d59: Open

suite result latest scheduled
astro failure failure
remix failure failure
sveltekit failure failure
vike failure failure
vite-environment-examples success failure
vite-plugin-svelte failure failure
vitest failure failure

analogjs, histoire, ladle, laravel, marko, nuxt, previewjs, quasar, qwik, rakkas, redwoodjs, storybook, unocss, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-vue, vite-setup-catalogue, vitepress, vuepress

@patak-dev
Copy link
Member

@sapphi-red I think we can merge this one, but letting you press the button because you added the p2-to-be-discussed label and I don't know if you'd like to bring this to a team meeting

@sapphi-red
Copy link
Member Author

I added the label because I thought it might be controversial whether this PR should be merged, since it can be seen as a configuration error. But if both of you think it's good to have, then I think we can merge it.

@patak-dev patak-dev merged commit 2ebe4b4 into vitejs:main Oct 26, 2024
14 checks passed
@sapphi-red sapphi-red deleted the fix/set-scripts-imported-by-html-module-sideeffects-true branch October 28, 2024 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-to-be-discussed Enhancement under consideration (priority)
Projects
None yet
3 participants