-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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(css): treeshake css modules #16051
Conversation
Run & review this pull request in StackBlitz Codeflow. |
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.
It'd be great if we can detect whether :global
is used and turn it off. But I don't find a way.
IIUC the behavior of the following case would change, but I think it's fine.
:global(.foo) {
color: red;
}
:global(.bar) {
color: green;
}
.baz {
color: blue;
}
import { baz } from './foo.module.css'
// doesn't use baz
/ecosystem-ci run |
This comment was marked as outdated.
This comment was marked as outdated.
Yeah. I think if they wrote it that way, the intent was probably also that if the CSS is not used, then the global CSS would also be not included. Still since we're changing it now they'll be a small risk. I also re-ran the |
📝 Ran ecosystem CI on
✅ analogjs, astro, histoire, ladle, laravel, marko, nuxt, previewjs, qwik, rakkas, sveltekit, unocss, vike, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-pages, vite-plugin-react-swc, vite-plugin-svelte, vite-setup-catalogue, vitepress, vitest |
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.
Nice one! Maybe we should merge this in Vite 5.2 just in case, but ok with doing it in a patch if you think it is safe enough.
Yeah I think we can merge it for 5.2. Not in a rush to get this out, mainly eyeing on #16058 instead 😄 |
The global CSS of the package will be Shaked, but I need these CSS. How should I handle them? |
You can use |
Yes, I can use layout.less instead. There is no reason to use .module.less that only contains global styles if you are not using its exported properties. |
I've got a bunch of But since updating vite to >5.2.0, all Is this a bug, or actually desired behaviour? And what can I do to fix it, apart from downgrading to vite 5.1.7 again? Thank you. Edit: It seems that all CSS vars declared in the Edit 2: I noticed that removing the |
@tobi-fis If the What this PR does is that if If you're seeing the case in the first paragaph, I'd appreciate if you can open a new issue with a repro. |
@bluwy It is actually the second case you described. So I guess this is desired behaviour now and we will have to adapt to it. |
Is there a configuration option to disable css tree-shaking? We're facing the breaking change mentioned in the original comment: We have some components that have all their styles within a Here's an example of the suboptimal workaround: tooltip.module.scss:
tooltip.tsx:
|
You can use this Vite plugin if you want to workaround it for now: function cssModuleSideEffectful() {
return {
name: 'css-module-side-effectful',
enforce: 'post'
transform(_, id) {
if (id.includes('.module.')) {
return {
moduleSideEffects: 'no-treeshake' // or true, which i think also works with slightly better treeshake
}
}
}
}
} |
Totally appreciate that the ship has sailed now and that it doesn't really make sense to have a After all, "css modules file containing only global styles" might be a bit silly but it's still valid per CSS modules, so the fact that this was only in the changelog as a bug fix caught my by surprise - I had a few applications that got quite spectacularly and hilariously broken by this one! |
Reading this thread I'm still stumped as to what the developers want me to do... :global(body) {
// styles here
} import "./globals.module.css"; This used to work in 5.1.7, broke in 5.2.0. I have no idea why dev mode will work, and the compiled wouldn't... this seems like a huge difference in developer expectations? Edit: I ended up just converting the file to plain CSS just so that I can move forward. |
Description
ref: #4389
I'm not sure if it's a big oversight, but we're able to treeshake CSS modules as long as we mark it side-effect-free. Because if the exported values were not used, it's also safe to remove the CSS completely.
Additional context
The only small breaking change is that if someone relied on
:global
classes to be included today, but this PR will treeshake it away. I think they should place it as a global CSS instead, so probably a non-issue (?)What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).