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: handle CSS Modules as modules #16018

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

privatenumber
Copy link
Contributor

@privatenumber privatenumber commented Feb 23, 2024

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 #15683
fixes #7504
fixes #10079
fixes #10340
fixes #14050 (this one is because I don't use Rollups dataToEsm)
fixes #16074
fixes #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:

  • getJSON
  • globalModulePaths
  • 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?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines, especially the Pull Request Guidelines.

  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.

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

  • Ideally, include relevant tests that fail without this PR but pass with it.

Copy link

stackblitz bot commented Feb 23, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

},
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests are removed because this plugin no longer handles CSS Modules.

)

expect(await imported.getAttribute('class')).toMatch(
/.composes-path-resolving-module__path-resolving-less___[\w-]{5}/,
/composes-path-resolving-module__path-resolving-less___[\w-]{5}/,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the . from these because they match wild-cards in Regex which are unnecessary here.

@patak-dev patak-dev added feat: css p3-significant High priority enhancement (priority) labels Feb 23, 2024
@patak-dev patak-dev added this to the 5.2 milestone Feb 23, 2024
@patak-dev
Copy link
Member

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

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@privatenumber
Copy link
Contributor Author

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

@patak-dev
Copy link
Member

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 #16011

cc @bluwy about the Astro question.

@privatenumber
Copy link
Contributor Author

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

@patak-dev

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@sapphi-red
Copy link
Member

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.

@privatenumber

This comment was marked as outdated.

@privatenumber
Copy link
Contributor Author

privatenumber commented Feb 28, 2024

@sapphi-red

I updated the hash template to be closer to postcss-modules (astro should now pass) and I also implemented localsConvention in privatenumber/vite-css-modules#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.

@sapphi-red

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@bluwy
Copy link
Member

bluwy commented Feb 28, 2024

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)

@sapphi-red
Copy link
Member

I'm also curious if we can explore the avenue of enabling arbitrary-module-namespace-names within Vite.

I think we shouldn't do that. Enabling it will cause browsers that does not support arbitrary module namespace names but specified in build.target to error in parse phase. That means the error cannot be caught in a normal way. Also users cannot notice that unless they run build + preview and check with the old browser.

By bumping the lowest modern chunk supported version / default build.target value, we can assume arbitrary module namespace names support. So after Vite 6, we can do that.

I can update to only enable string export names based on build.target

I wonder if that is a bit too magical. But I'm still fine with it. I guess we can check it by whether esbuild throws an error.

@privatenumber
Copy link
Contributor Author

@bluwy
Copy link
Member

bluwy commented Mar 13, 2024

Moving this to the 5.3 milestone. We'd like to keep 5.2 as a small release for now, and keep the larger changes to 5.3. We also need to discuss this approach in the next team meeting before going forward. Personally for me, I'd like to see some of my concerns here addressed.

@bluwy bluwy modified the milestones: 5.2, 5.3 Mar 13, 2024
@privatenumber
Copy link
Contributor Author

privatenumber commented Mar 13, 2024

@bluwy Thanks for reminding me. Here are my responses inline:

I guess I was thinking postcss-modules could expose a separate technique where it can assume a bundler in the pipeline.

Would like to move away from using postcss-modules for the following reasons:

  1. It's not maintained. Last commit was 2 years ago.

  2. It caters to a fundamentally different use-case. It was not designed to work with bundlers, and trying to make it do so has led to the issues this PR is trying to fix.

  3. It has APIs that don't make sense in the context of using a bundler (e.g. getJSON, globalModulePaths, etc ). For the next major, I'm excited to discuss removing many of them to simplify how CSS Modules is implemented.

  4. Even if we can make changes to it, the breaking changes would disrupt current users who are using it as intended (e.g. without a bundler).

  5. The implementation is convoluted and comes at a high maintenance cost. It's a PostCSS plugin that implements a half-baked bundler which processes each module by calling PostCSS again. This is akin to a Babel plugin that calls Rollup with the Rollup Babel plugin—it feels very hacky. (I'm sure there are reasons for it, but the takeaway here is that it's not suited for Rollup.)

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.

Yeah, the core packages released by the CSS Modules team are PostCSS plugins so it's necessary. But in Lightning mode, it doesn't use PostCSS.

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.

Ah yeah, if preprocessCss is a public API (don't see it documented), it would be breaking. But I think this is how it should be. I think part of the fix and predictability of this new approach was moving CSS Modules from pre-processing to post-processing. Deferring the CSS Module compilation to the final step will make it easier for other CSS plugins (e.g. 3rd party) to insert itself into the pipeline.

Essentially, compiling CSS Modules in the pre-processing step was probably not the best decision.

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

I think it's better to abstract CSS Module compilation into its own plugin. It processes CSS, but it's quite different from other CSS processors in that it outputs JS.

Kind of like with SVGs... there are SVG optimization plugins and there are SVG-to-JS plugins, but you wouldn't want to couple them together as converting the SVG to JS (SVG string or an asset URL) is really an interfacing concern rather than a processing one.

It also has quite a bit of complexity, and it even returns a source map. Separating the concerns will improve maintainability & testability (avoiding shoe-horning tests like this).

@bluwy
Copy link
Member

bluwy commented Mar 13, 2024

Thanks for taking the time to respond! Your explanation about postcss-modules makes sense to me. I agree that some of its API doesn't quite fit for bundlers and we could revisit them in the next major. If we do move to a new package, I'd be careful to not dismiss its contribution to Vite and the ecosystem though. The author may have their own reasons.


Re preprocessCss, I can see that handling CSS modules isn't technically a preprocess step, so if the rest are fine with this breaking change, we can go ahead instead.


I think making CSS modules its own plugin and as a post plugin makes sense. My remaining concern is that I'm not sure if this PR's implementation (import { cssModules } from 'vite-css-modules') is the right abstraction or "the cut-off point between Vite and third-party package".

I'd prefer if the CSS modules plugin is written in Vite instead, like mentioned by patak and sapphi. But not entirely inlining vite-css-modules inside Vite, rather, we keep the same relationship as postcss-modules. e.g. Use a third-party package to do the heavy lifting generically, and create a Vite plugin to link everything up. For example, if it can expose

// rough idea (not exactly proposing this API)
function compileCSSModule(css: string, options: PostcssModulesConfig | LightningcssModulesConfig):
  Promise<{ code: string, css: string, exports: Record<string, any>, map: SourceMap }>

That way we can own the CSS modules plugin, and the heavy-lifting/tests are done third-party. I'd hold off implementing this first though to hear what @patak-dev and @sapphi-red think about this. If they prefer to inline everything into Vite, then I can't argue with that too 😅

@privatenumber
Copy link
Contributor Author

If we do move to a new package, I'd be careful to not dismiss its contribution to Vite and the ecosystem though. The author may have their own reasons.

Assuming you're referring to Reason # 6, it's in response to your suggestion to contribute upstream.

Reasons for complexity aside, I'm pointing out that adopting their approach comes with a high maintenance cost & engineering complexity that isn't necessary when using with Rollup. I didn't speak to their contribution to Vite + the ecosystem, but I edited the comment for clarity.

I'd prefer if the CSS modules plugin is written in Vite instead [...] but not entirely inlining vite-css-modules inside Vite, rather, we keep the same relationship as postcss-modules. e.g. Use a third-party package to do the heavy lifting generically, and create a Vite plugin to link everything up. For example, if it can expose

// rough idea (not exactly proposing this API)
function compileCSSModule(css: string, options: PostcssModulesConfig | LightningcssModulesConfig):
  Promise<{ code: string, css: string, exports: Record<string, any>, map: SourceMap }>

I had this idea originally as well, and it's kind of how I implemented it in vite-css-module. In the end, I didn't make it a separate package because there was too much coupling and also too little code to make a new package for.

The plugin calls transform, which is your compileCSSModule function, and that can be either LightningCSS or PostCSS. CSSModule compilation via PostCSS is basically just configuration. The rest of the logic is normalizing the result to match LightningCSS's output so the transformers are interchangeable (which is a concern tightly coupled to this plugin). It can be slimmed down a lot more once we remove the unnecessary options, and in that state, there wouldn't be much logic to abstract out.


As I said earlier, I'm OK pulling in vite-css-module to Vite. But since the all tests are passing now, could I ask for it to be done in a follow up PR at a later date?

(I'm happy to collaborate long-term on this, but just to share my perspective: I've been working on this PR a lot longer than I hoped, and with the workaround, it's hard to keep this priority longer. Giving back to Vite is my primary motivation but the ongoing requests without a clear end-goal can feel unrewarding. If possible, I would love to collaborate incrementally.)

@patak-dev
Copy link
Member

@privatenumber, we really appreciate you looking into this problem and all the work you already have done here. It will be a big improvement for everyone. This is a big change though. I understand that you feel this was going to be easier to get merged but the discussion so far has been conducted well for the way Vite works and IMO is required to land this correctly with no disruption to the ecosystem and with our eyes on the long term maintainability of the project. I hope you don't get too frustrated by what is the regular flow when adding a new dependency, and we can keep working together. We already decided that this is too big of a change for Vite 5.2, as we want to release the next minor quickly and then focus bigger changes in 5.3 (that currently only have two PRs, this one and the Environment API). Vite 5.3 will be released in around 2 months so there is no rush at this point.

Given the discussion above, I'm leaning to pulling in vite-plugin-css to Vite as being the best option. I hope that could happen for Vite 5.3, if you'd find that a good idea too. I'd like to also hear what @sapphi-red thinks too before though.

@privatenumber
Copy link
Contributor Author

privatenumber commented Mar 13, 2024

@patak-dev

Thanks for letting me know the target release is 2 months away—I will plan to follow up with the refactor in a few weeks.

To clarify, I'm not asking for it to be released (I'm unblocked via my plugin). Just want to ask for the work to be divided into incremental PRs. Specifically, it would be helpful if there could be a feature branch within Vite, allowing me to merge this PR and open follow-up PRs to it. I can open follow-up PRs now but it would be within my fork.

Also, I think we've reached a consensus on pulling vite-plugin-css into Vite. I think @bluwy was asking about the implementation detail of whether to abstract out the compilation step to a util package, but I explained the concerns are too coupled.

@patak-dev
Copy link
Member

We have the same issue with the Environment API. What we are doing with it is have an umbrella PR (see #16129), where we are pushing changes from other PRs (see #16137). We also commit directly to it when we don't need a separate space to discuss or trigger ecosystem-ci. You could send a new PR, point this one to it and merge it. And then start sending new PRs against it until it is ready.
Rolldown is now using https://graphite.dev/blog/stacked-prs. We'll discuss with them to see how it is working after a while. Maybe we could adopt something similar.

@bluwy
Copy link
Member

bluwy commented Mar 14, 2024

@privatenumber I figured to draft my idea in a local branch to showcase the abstraction I was thinking of. I have a modified version of vite-css-modules under packages/css-modules which I don't mind if you want to port some things back to your package.

I thought it was a quick 30min sketch, but I've spent several hours now polishing it up 😅 It kinda works but tests are failing, but I'm not pushing for the branch over-the-line, only mainly to get the idea of the state of this PR so far. The bright side is that I understand some aspects of vite-css-modules like loadExports, fallback load(), ?.module.css, exportAs set, getJSON/.resolved etc and how postcss-modules options made it complex 😄

Happy to collaborate on it though if you'd like, or help take over if the PR is taking a while. Like patak mentioned, we'd like a get it right when making a big change to Vite as we're depended by many frameworks.

Note: In the branch, I didn't make CSS modules its own plugin. I discovered that that causes lightningcss to run twice, which I'm not sure is a good default for perf. So I moved the CSS modules handling to compileCSS, and some inside css-post plugin, mainly making dataToEsm more powerful.

@privatenumber
Copy link
Contributor Author

@bluwy

Wow thank you so much 🙌
I appreciate you trying to better understand the problems by getting involved.

Happy to collaborate! Do you have a preferred way for us to chat?

I'm determined to get this right as well. And I'm not pressed to get this released because of my plugin.
I just wanted to merge to a local branch like you have (Stacked PR approach patak mentioned).


@patak-dev

Sorry I'm not sure if I follow. Which branch do you want me to change this PR against?

@patak-dev
Copy link
Member

Sorry I'm not sure if I follow. Which branch do you want me to change this PR against?

@bluwy maybe you could create a new feat/revamped-css-modules (it can start from main and you PR it without any changes to have the PR to discuss) and then this PR can be merged against it and used as a base for future work? Future PRs can be made against that branch until the feature is ready to be merged.

@bluwy
Copy link
Member

bluwy commented Mar 15, 2024

Happy to chat about it at the Vite Discord server #contributing channel! I imagine there will be a lot of back-and-forth, so that will help here.

I think personally I'd hold off having a feature branch to merge in first. I think there's some topics to talk about from this current implementation, like "post-load fallback", "css modules as post-plugin", "different abstraction api", etc. That will be hard to change from a feature branch if we merge it in. The code may look very different after subsequent PRs, but a feature branch is still definitely on-the-table once we have a good direction of the implementation later.

bluwy pushed a commit that referenced this pull request Mar 18, 2024
@hi-ogawa
Copy link
Collaborator

hi-ogawa commented Mar 19, 2024

I only went through the surface, but the idea makes sense to me. Awesome work!

Just in case, let me mention Remix's use case of css module since it might not be covered by ecosystem ci.
I think what Remix does goes like this:
(the approach is based on solid-start at that time, but it looks like similar code now lives in vinxi.)

collect css module's css by intercepting `transform` hook

https://github.com/remix-run/remix/blob/6ad886145bd35298accf04d43bd6ef69833567e2/packages/remix-dev/vite/plugin.ts#L1269-L1272
https://github.com/nksaraf/vinxi/blob/00d68f0e9711b44cc5e011588e52803bf8160b91/packages/vinxi/lib/plugins/css.js#L33-L37

      async transform(code, id) {
        if (isCssModulesFile(id)) {
          cssModulesManifest[id] = code;
        }
include css module's css code in SSR style head to avoid FOUC

https://github.com/remix-run/remix/blob/6ad886145bd35298accf04d43bd6ef69833567e2/packages/remix-dev/vite/styles.ts#L77-L79
https://github.com/nksaraf/vinxi/blob/00d68f0e9711b44cc5e011588e52803bf8160b91/packages/vinxi/lib/manifest/collect-styles.js#L173

        let css = isCssModulesFile(dep.file)
          ? cssModulesManifest[dep.file]
          : (await viteDevServer.ssrLoadModule(dep.url)).default;

Does this idea still work with new js module centric approach or does it need to be written differently?
Or maybe is there entirely different alternative if astro, rakkas, etc... are not using this transform interception approach?

Hopefully I'm not missing what Remix actually does. Maybe it's better to confirm with Remix and Vinxi side.


EDIT:
A little more background. I was actually testing css modules on my framework hi-ogawa/vite-plugins#205 and was considering to borrow some ideas from Remix.

I knew about ?direct request and also using ssrLoadModule felt a little odd, so what I ended up is to use transformRequest(dep.url + "?direct") instead of ssrLoadModule and it seems to work for both normal css and css modules. Maybe this is a way to go instead of keeping track of css module's css via transform hook?

@bluwy
Copy link
Member

bluwy commented Mar 19, 2024

Thanks for sharing @hi-ogawa. Yeah since this PR moves the CSS modules handling as a post-plugin, I think that usecase might cause cssModulesManifest[id] = code; to get the incorrect CSS (which is before CSS modules is processed).

Your edit seems like a good way around it, I would agree that retrieving the CSS for SSR currently is quite clunky, because Vite's API is made around the assumption that files are lazily processed. For reference, here's how Astro (uses links) and SvelteKit (uses ?inline) handle CSS modules FOUC.

Since there's an implementation today that relies on how CSS modules are processed, I wouldn't mind moving the CSS modules handling back as a normal plugin to not break this for now, and in the next major, we could move it as a post plugin.


Also, I've talked with privatenumber recently about where to go from here. I've created a new feat/revamped-css-modules feature branch that's squash-merged from this PR and we'll continue iterating from there. I'll be helping around to get this feature finished by Vite 4.3.

@snake-py
Copy link

Hey just comming around this issue, will this PR also solve the issue that you get a full page reload on an update of a module.css file? I first thought that this is a vike issue, but it seems to be a vite server render issue?

@privatenumber
Copy link
Contributor Author

privatenumber commented Aug 16, 2024

I'm not sure what problem you're describing, but this fix is abstracted in the https://github.com/privatenumber/vite-css-modules plugin so you can confirm yourself.

The work in this PR has paused. These days, my opinion is that CSS Modules should be a 2nd party plugin.

@bluwy
Copy link
Member

bluwy commented Oct 29, 2024

An update on my end: I really wanted to pick this back up one day but there's a lot still to get done and verifying that it's compatible to as before (as close as possible). Among the few things I want to check:

  1. Last we discussed, there's slight differences in how the "imported" CSS modules variables were inlined. This affected Vite's builtin resolving to be able to retrieve the referenced file, but we probably don't want this resolving behaviour exposed.
  2. Making CSS and CSS modules handling a separate two-step process doesn't work well for lightningcss. It wants to control the entire transform and do it once. Splitting it out has caveats, like invalid CSS-modules-specific syntax parsing if css modules option is not turned on, and potentially perf implications. We need to figure out some consistency or tradeoffs here.
  3. I wanted a new internal package for this different CSS modules handling. We could punt that off, but ideally still have the code modularize enough so that the domain-specific logic is all located in one place.
    I'm still a bit worried about integrating too many domain-specific code for CSS modules here. Part of how Vite is structured is that outsources well-scoped work as necessary, e.g. rollup, esbuild, postcss, postcss-import, plugin-commonjs, etc.

Sorry for causing the PR to be stale. The chores laying in front here has caused me to not prioritize this in a while 😅

@patak-dev patak-dev removed this from the 6.0 milestone Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment