vite icon indicating copy to clipboard operation
vite copied to clipboard

fix: handle CSS Modules as modules

Open privatenumber opened this issue 1 year ago • 50 comments

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.

privatenumber avatar Feb 23 '24 08:02 privatenumber

Review PR in StackBlitz Codeflow 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.

patak-dev avatar Feb 23 '24 09:02 patak-dev

/ecosystem-ci run

patak-dev avatar Feb 23 '24 09:02 patak-dev

📝 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

vite-ecosystem-ci[bot] avatar Feb 23 '24 09:02 vite-ecosystem-ci[bot]

  • 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 localsConvention yet (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.

privatenumber avatar Feb 23 '24 15:02 privatenumber

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.

patak-dev avatar Feb 23 '24 15:02 patak-dev

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

privatenumber avatar Feb 23 '24 15:02 privatenumber

/ecosystem-ci run

patak-dev avatar Feb 23 '24 16:02 patak-dev

📝 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

vite-ecosystem-ci[bot] avatar Feb 23 '24 16:02 vite-ecosystem-ci[bot]

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.

sapphi-red avatar Feb 27 '24 08:02 sapphi-red

/ecosystem-ci run

privatenumber avatar Feb 28 '24 11:02 privatenumber

@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.

privatenumber avatar Feb 28 '24 11:02 privatenumber

/ecosystem-ci run

sapphi-red avatar Feb 28 '24 12:02 sapphi-red

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 avatar Feb 28 '24 13:02 bluwy

@bluwy

If vite-css-modules could 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?

privatenumber avatar Feb 29 '24 00:02 privatenumber

/ecosystem-ci run

patak-dev avatar Feb 29 '24 06:02 patak-dev

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 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.

bluwy avatar Feb 29 '24 06:02 bluwy

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 {} 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.

To handle this problem, I think we need a new option to make the feature (#14050) opt-in.

sapphi-red avatar Mar 01 '24 11:03 sapphi-red

@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.

privatenumber avatar Mar 06 '24 08:03 privatenumber

/ecosystem-ci run

sapphi-red avatar Mar 06 '24 09:03 sapphi-red

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?

privatenumber avatar Mar 06 '24 10:03 privatenumber

@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

privatenumber avatar Mar 07 '24 02:03 privatenumber

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.

sapphi-red avatar Mar 07 '24 10:03 sapphi-red

rakkas should pass now :)

Thanks for helping @sapphi-red

privatenumber avatar Mar 08 '24 04:03 privatenumber

/ecosystem-ci run

sapphi-red avatar Mar 09 '24 11:03 sapphi-red

@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

sapphi-red avatar Mar 09 '24 11:03 sapphi-red