fix: handle CSS Modules as modules
Description
This PR changes how CSS Modules are handled in Vite.
Previously, CSS Modules were bundled at every entry-point via postcss-modules. This caused issues, duplicating CSS Module dependencies, and preventing Vite plugins from accessing dependencies.
With this PR, CSS Modules are now resolved & bundled by Vite/Rollup by compiling them into individual JS modules.
fixes https://github.com/vitejs/vite/issues/15683
fixes https://github.com/vitejs/vite/issues/7504
fixes https://github.com/vitejs/vite/issues/10079
fixes https://github.com/vitejs/vite/issues/10340
fixes https://github.com/vitejs/vite/issues/14050 (this one is because I don't use Rollups dataToEsm)
fixes https://github.com/vitejs/vite/issues/16074
fixes https://github.com/vitejs/vite/issues/16075
To achieve this, I created a plugin that handles CSS Modules here: https://github.com/privatenumber/vite-css-modules
I realize this approach is somewhat unconventional, but I wanted a way to offer this change both as a patch for users unable to upgrade Vite, and as an integration into Vite core.
It was also easier to scope this logic in a separate repo with tests. And since the previous CSS Modules implementation was an external package, I figured this isn't too far fetched.
Additional context
LightningCSS is ready to go, but I haven't implemented full parity with postcss-modules yet:
- [x] getJSON
- [x] globalModulePaths
- [x] localsConvention
TBH I think these options should be removed in the future, but I can implement them for parity if this is something we want.
What is the purpose of this pull request?
- [x] Bug fix
- [ ] New Feature
- [ ] Documentation update
- [ ] Other
Before submitting the PR, please make sure you do the following
-
[x] Read the Contributing Guidelines, especially the Pull Request Guidelines.
-
[x] Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
-
[x] Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g.
fixes #123). -
[ ] Update the corresponding documentation if needed.
I have not updated documentation yet. Hoping to receive feedback first.
-
[x] Ideally, include relevant tests that fail without this PR but pass with it.
Run & review this pull request in StackBlitz Codeflow.
Nice! Thanks for working on this @privatenumber. I'd like to see what other maintainers think about the separate package. My first impression is that CSS Modules are important enough for this plugin to live directly in Vite core. It will be good to avoid sync issues, for example with internals of Vite you are duplicating.
/ecosystem-ci run
📝 Ran ecosystem CI on dc5834f: Open
| suite | result | latest scheduled |
|---|---|---|
| analogjs | :white_check_mark: success | :white_check_mark: success |
| astro | :x: failure | :white_check_mark: success |
| histoire | :white_check_mark: success | :white_check_mark: success |
| ladle | :white_check_mark: success | :white_check_mark: success |
| laravel | :white_check_mark: success | :white_check_mark: success |
| marko | :white_check_mark: success | :white_check_mark: success |
| nuxt | :white_check_mark: success | :white_check_mark: success |
| nx | :x: failure | :x: failure |
| previewjs | :white_check_mark: success | :white_check_mark: success |
| qwik | :white_check_mark: success | :white_check_mark: success |
| rakkas | :x: failure | :white_check_mark: success |
| sveltekit | :white_check_mark: success | :white_check_mark: success |
| unocss | :x: failure | :white_check_mark: success |
| vike | :white_check_mark: success | :white_check_mark: success |
| vite-plugin-pwa | :white_check_mark: success | :white_check_mark: success |
| vite-plugin-react | :white_check_mark: success | :white_check_mark: success |
| vite-plugin-react-pages | :white_check_mark: success | :white_check_mark: success |
| vite-plugin-react-swc | :white_check_mark: success | :white_check_mark: success |
| vite-plugin-svelte | :white_check_mark: success | :white_check_mark: success |
| vite-plugin-vue | :x: failure | :white_check_mark: success |
| vite-setup-catalogue | :white_check_mark: success | :white_check_mark: success |
| vitepress | :white_check_mark: success | :white_check_mark: success |
| vitest | :white_check_mark: success | :white_check_mark: success |
-
astro is failing because it expects the exact CSS Module hash. I'm not sure if it's expected or necessary for the hashed class to be deterministic. What do you think?
-
vite-plugin-vue is failing because I didn't implement
localsConventionyet (importing kebab-case classnames as camelCase). Will look into this.
Currently not sure why nx, rakkas, and unocss are failing—it doesn't seem related to CSS Modules but I'll dig deeper later.
nx is already failing in main. I think you need to merge main into this PR and the unocss and rakkas issues will be gone. See https://github.com/vitejs/vite/pull/16011
cc @bluwy about the Astro question.
Gotcha, thanks! Hope it passes now 🙏
Astro is failing because it basically hard-coded the expected hash.
postcss-modules uses this as the default hash:
https://github.com/madyankin/postcss-modules/blob/v6.0.0/src/scoping.js#L41
But I thought it was needlessly complex and simplified it to: https://github.com/privatenumber/vite-css-modules/blob/0a20d9e1748f4c344e41207f8b43d6984460d342/src/plugin/transformers/postcss/index.ts#L21
/ecosystem-ci run
📝 Ran ecosystem CI on b914553: Open
| suite | result | latest scheduled |
|---|---|---|
| analogjs | :x: failure | :white_check_mark: success |
| astro | :x: failure | :white_check_mark: success |
| histoire | :white_check_mark: success | :white_check_mark: success |
| ladle | :white_check_mark: success | :white_check_mark: success |
| laravel | :white_check_mark: success | :white_check_mark: success |
| marko | :white_check_mark: success | :white_check_mark: success |
| nuxt | :white_check_mark: success | :white_check_mark: success |
| nx | :x: failure | :x: failure |
| previewjs | :white_check_mark: success | :white_check_mark: success |
| qwik | :white_check_mark: success | :white_check_mark: success |
| rakkas | :x: failure | :white_check_mark: success |
| sveltekit | :white_check_mark: success | :white_check_mark: success |
| unocss | :white_check_mark: success | :white_check_mark: success |
| vike | :white_check_mark: success | :white_check_mark: success |
| vite-plugin-pwa | :white_check_mark: success | :white_check_mark: success |
| vite-plugin-react | :white_check_mark: success | :white_check_mark: success |
| vite-plugin-react-pages | :white_check_mark: success | :white_check_mark: success |
| vite-plugin-react-swc | :white_check_mark: success | :white_check_mark: success |
| vite-plugin-svelte | :white_check_mark: success | :white_check_mark: success |
| vite-plugin-vue | :x: failure | :white_check_mark: success |
| vite-setup-catalogue | :white_check_mark: success | :white_check_mark: success |
| vitepress | :white_check_mark: success | :white_check_mark: success |
| vitest | :white_check_mark: success | :white_check_mark: success |
Awesome!
I prefer having it directly in Vite core for the same reason with @patak-dev.
(this one is because I don't use Rollups
dataToEsm)
You can use includeArbitraryNames: true option for that.
But generating an export using arbitrary names feature will make it impossible to transpile the bundle to lower versions. For example, if you add .if {} in playground/css/mod.module.css and make that CSS file in a separate chunk (e.g. dynamic import that file), an error will happen.
TBH I think these options should be removed in the future, but I can implement them for parity if this is something we want.
getJSON and globalModulePaths are rarely used, but localsConvention is used a lot.
I think we need to support localsConvention at least.
I'm not sure if setting globalModulePaths makes sense in the first place. Should this option be set as /(?<!\.module)\.css$/ so that it follows our naming convention?
If it's not complicated, I think we should support getJSON.
/ecosystem-ci run
@sapphi-red
I updated the hash template to be closer to postcss-modules (astro should now pass) and I also implemented localsConvention in https://github.com/privatenumber/vite-css-modules/pull/1 (vite-plugin-vue should now pass) But I think it's made it harder to leverage dataToEsm. I saw you implemented includeArbitraryNames 🙏 but there was a different reason (can't remember...) I couldn't use it.
I agree both getJSON & globalModulePaths should be implemented for feature parity. We should discuss slimming down the API in the next major.
As for the plugin, I do agree it would be nice to have CSS Modules in core, and I'm happy to pull it in this PR when ready.
I think moving the plugin in will be straightforward, but I think adding the tests will take time for me. Could this be done in a follow-up PR? I do want to point out that Vite previously relied on an external package for CSS Modules (postcss-modules), so there isn't difference there.
Would you mind running ecosystem-ci? I think most tests will pass now with the exception of some source map tests.
Will implement getJSON & globalModulePaths soon.
/ecosystem-ci run
📝 Ran ecosystem CI on d936295: Open
| suite | result | latest scheduled |
|---|---|---|
| nx | :x: failure | :x: failure |
| rakkas | :x: failure | :white_check_mark: success |
| vite-plugin-vue | :x: failure | :x: failure |
:white_check_mark: analogjs, astro, histoire, ladle, laravel, marko, nuxt, previewjs, qwik, 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
Late to the party. If the Astro fail is only because of hash mismatch, I think we can fix that on the Astro side.
About the PR, this is an interesting approach. I don't think we necessarily have to move the entire plugin to core, as I don't think we want to maintain a duplicate CSS modules handling similar to postcss-modules? If vite-css-modules could provide an abstraction that's perhaps more generic, then I think we could keep that as a separate library. I think webpack could also benefit from this.
However, If we keep it that way, maybe it makes more sense to upstream this feature directly to postcss-modules? (if it makes sense)
@bluwy
If
vite-css-modulescould provide an abstraction that's perhaps more generic
The generic parts of CSS Modules are already provided as packages by the CSS Modules team (same packages that Webpack's css-loader uses).
But, if I were to implement CSS Modules as a Rollup plugin, I'd do it differently so it's more compatible with other plugins to transform the CSS. But for the purposes of patching Vite's CSS Modules behavior without breaking change, I was constrained to implement it this way. That said, this plugin is only compatible with Vite. I decided to implement it as a separate package first because the primary purpose of the plugin is for others to use/test this fix without upgrading Vite or waiting for this PR.
For the next major, I would love to make a proposal towards how CSS Modules should be implemented as a Rollup plugin. I've even secured the rollup-plugin-css package name :)
However, If we keep it that way, maybe it makes more sense to upstream this feature directly to postcss-modules? (if it makes sense)
Can't because postcss-modules targets a different use-case where there is no other bundler in the pipeline. It implements its own bundler, creating a black-boxed CSS Module bundle separate from Rollups, which leads to the bugs (e.g. duplicated dependencies, other Vite plugins not being able to access composed CSS Modules).
I've implemented getJSON and globalModulePaths. This PR should be ready!
Can you run ecosystem-ci?
/ecosystem-ci run
📝 Ran ecosystem CI on d78a93e: Open
| suite | result | latest scheduled |
|---|---|---|
| nx | :x: failure | :x: failure |
| rakkas | :x: failure | :white_check_mark: success |
| vite-plugin-vue | :x: failure | :x: failure |
:white_check_mark: analogjs, astro, histoire, ladle, laravel, marko, nuxt, previewjs, qwik, 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
Can't because
postcss-modulestargets a different use-case where there is no other bundler in the pipeline. It implements its own bundler, creating a black-boxed CSS Module bundle separate from Rollups, which leads to the bugs (e.g. duplicated dependencies, other Vite plugins not being able to access composed CSS Modules).
I guess I was thinking postcss-modules could expose a separate technique where it can assume a bundler in the pipeline. The name itself doesn't mean it has to be a PostCSS plugin, plus reading the alternative implementation right (vite-css-modules), it still relies on postcss to process internally. But of course if it makes sense as a separate package, that's also good as an exploration phase.
Correct me if I'm wrong, but I'm thinking an abstraction could look like that after we call compileCSS() on the CSS module, we could use an API to later process those and return the final CSS and JS? Something like:
const { code: css } = await compileCSS(id, css)
if (isModule) {
const { code: newCss, js } = await compileCSSModule(css, modulesConfig) // uses postcss or lightningcss
// ...
}
Also, I think this PR breaks the experimental preprocessCss API as it no longer returns a module, but I've personally not seen usage that relies on this. If we want to fix this, we have to move the CSS modules handling inside compileCSS.
It seems plugin-vue is failing because the source map is broken. sourcesContent should be the content of the vue file but it's an empty string.
https://github.com/vitejs/vite-ecosystem-ci/actions/runs/8091723574/job/22111257321#step:8:871
But generating an export using arbitrary names feature will make it impossible to transpile the bundle to lower versions. For example, if you add
.if {}inplayground/css/mod.module.cssand make that CSS file in a separate chunk (e.g. dynamic import that file), an error will happen.
To handle this problem, I think we need a new option to make the feature (#14050) opt-in.
@sapphi-red Just implemented source map support. Would you mind running ci again?
But generating an export using arbitrary names feature will make it impossible to transpile the bundle to lower versions. For example, if you add .if {} in playground/css/mod.module.css and make that CSS file in a separate chunk (e.g. dynamic import that file), an error will happen.
I haven't looked into this yet, but will look into it soon.
/ecosystem-ci run
📝 Ran ecosystem CI on 4261c58: Open
| suite | result | latest scheduled |
|---|---|---|
| analogjs | :white_check_mark: success | :x: failure |
| nx | :x: failure | :x: failure |
| rakkas | :x: failure | :white_check_mark: success |
:white_check_mark: astro, histoire, ladle, laravel, marko, nuxt, previewjs, qwik, sveltekit, unocss, vike, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-pages, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress, vitest
CI / Build&Test: node-20, ubuntu-latest is failing due to a WebSocket error. I think re-reunning should pass it.
It seems both nx and analogjs are failing on main. I'm not sure if I understand the rakkas errors—is it relevant to these changes?
@sapphi-red
But generating an export using arbitrary names feature will make it impossible to transpile the bundle to lower versions. For example, if you add .if {} in playground/css/mod.module.css and make that CSS file in a separate chunk (e.g. dynamic import that file), an error will happen.
I looked into this, and it doesn't seem like a problem in my approach: https://stackblitz.com/edit/vitejs-vite-hpdg7d?file=src%2Fmain.js
Likely because I only declare safe variable names via Rollup's makeLegalIdentifier.
I do something like:
const safeIf = 'hash';
export { safeIf as 'if' }
This is optimized so it's only done if the safe version of the name is different from the original: https://github.com/privatenumber/vite-css-modules/blob/c02aefe58d9343058c3e0c738716567b3ffa0903/src/plugin/generate-esm.ts#L42-L44
I'm not sure if I understand the rakkas errors—is it relevant to these changes?
I guess it is. IIUC this PR changes the generated module graph and that might be affecting rakkas. I'm not sure which side the fix should be done on though. I guess https://github.com/rakkasjs/rakkasjs/commit/73f218d24e2879ad3bdf836bf04aa9b584c7106b is the related commit.
rakkas should pass now :)
Thanks for helping @sapphi-red
/ecosystem-ci run
@sapphi-red
But generating an export using arbitrary names feature will make it impossible to transpile the bundle to lower versions. For example, if you add .if {} in playground/css/mod.module.css and make that CSS file in a separate chunk (e.g. dynamic import that file), an error will happen.
I looked into this, and it doesn't seem like a problem in my approach: https://stackblitz.com/edit/vitejs-vite-hpdg7d?file=src%2Fmain.js
Likely because I only declare safe variable names via Rollup's
makeLegalIdentifier.I do something like:
const safeIf = 'hash'; export { safeIf as 'if' }This is optimized so it's only done if the safe version of the name is different from the original: https://github.com/privatenumber/vite-css-modules/blob/c02aefe58d9343058c3e0c738716567b3ffa0903/src/plugin/generate-esm.ts#L42-L44
Here's an example that fails (a class name including -): https://stackblitz.com/edit/vitejs-vite-ccwcai?file=package.json
📝 Ran ecosystem CI on 67e93c8: Open
| suite | result | latest scheduled |
|---|---|---|
| analogjs | :white_check_mark: success | :x: failure |
:white_check_mark: 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-plugin-vue, vite-setup-catalogue, vitepress, vitest