search

vitejs / vite

Next generation frontend tooling. It's fast!
http://vitejs.dev
MIT License
66.28k stars 5.92k forks source link

CSS Modules - composed styles are duplicated #7504

Open desmond-tuiyot opened 2 years ago

desmond-tuiyot commented 2 years ago

Describe the bug

Importing a css module that's been composed ends up duplicating styles.

In the reproduction linked below, our App.tsx has 2 div elements with texts that have different styles. The styles are under the styles folder. The first text is styled using originalText style from original.module.css, and should have a 32px font size. The second one should have a 14px font size, since it's styled using smallText from small.module.css, which composes originalText and then overrides the font size. However, it doesn't work as expected.

The originalText style from original.module.css is imported first, then smallText style is imported, then the originalText style is imported once again, overriding the font size provided by smallText.

You can see this in action if you inspect the div element in dev tools and check out the styles applied.

Reproduction

https://codesandbox.io/s/vite-react-ts-forked-dy91of?file=/src/App.tsx

System Info

System:
    OS: macOS 12.2.1
    CPU: (8) arm64 Apple M1
    Memory: 127.45 MB / 16.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 16.13.2 - /usr/local/bin/node
    Yarn: 1.22.17 - /opt/homebrew/bin/yarn
    npm: 8.1.2 - /usr/local/bin/npm
  Browsers:
    Brave Browser: 99.1.36.117
    Chrome: 99.0.4844.83
    Firefox: 97.0.1
    Safari: 15.3
  npmPackages:
    @vitejs/plugin-react: ^1.2.0 => 1.2.0 
    vite: ^2.8.6 => 2.8.6

Used Package Manager

npm

Logs

No response

Validations

ygj6 commented 2 years ago

I'm not particularly familiar with CSS Modules, but it works fine when you change their import order.

import originalStyles from './styles/original.module.css';
import smallStyles from './styles/small.module.css'; // import later for higher priority
desmond-tuiyot commented 2 years ago

I'm not particularly familiar with CSS Modules, but it works fine when you change their import order.

import originalStyles from './styles/original.module.css';
import smallStyles from './styles/small.module.css'; // import later for higher priority

In this small example it does have a workaround. But when I'm composing classes across many different css modules in my actual project, I have no way of controlling the import order myself. That's entirely in vite territory.

ygj6 commented 2 years ago

@desmond-tuiyot You are right, I tested it in the webpack project with no problems

xiaoxian521 commented 2 years ago

I also have the same problem

fi3ework commented 2 years ago

css-modules indicates that the order is not granted when composing from multiple files. So maybe the best practice is to split the properties into different files. https://github.com/css-modules/css-modules#composing-from-other-files

Note that when composing multiple classes from different files the order of appliance is undefined. Make sure to not define different values for the same property in multiple class names from different files when they are composed in a single class.

desmond-tuiyot commented 2 years ago

css-modules indicates that the order is not granted when composing from multiple files. So maybe the best practice is to split the properties into different files. https://github.com/css-modules/css-modules#composing-from-other-files

I think that that disclaimer applies to situations when you're composing multiple classes into one style rule. For example

 a {
    composes: b from './b.module.css';
    composes: c from './c.module.css';
  }
}

My case is different. And also in my actual project, I'm not importing 2 different css module into one file. This example is set up that way for simplicity

sapphi-red commented 2 years ago

I think this problem cannot be solved with Vite due to the architecture. Vite processes each css import individually.

To dedupe .originalText, it will need to process all css files at once because it needs to know depency graph.

One way it may solve this is to rewrite composes into a import with JS. But I feel this is no longer "CSSModules".

ygj6 commented 2 years ago

FYI, https://github.com/madyankin/postcss-modules/issues/32

axelboc commented 1 year ago

In https://github.com/vitejs/vite/issues/5185, I reported an ordering issue that @sapphi-red rightfully identified as a consequence of this duplication issue.

In my case, I was looking at a compiled CSS output, which did not have duplicated styles. I assume this is because cssnano removes the duplicated styles at build time?

If so, then my ordering issue was most likely caused by cssnano keeping only the last occurrence of a duplicated style. Am I wrong in thinking that by keeping only the first occurrence, the issue would be resolved for the most part? This solution probably has edge cases, but it could be a decent way forward.

If this alternative deduplication behaviour cannot be implemented in cssnano, could it be done by a dedicated Vite plugin? Maybe this plugin could even work in development to remove duplicated styles dynamically?

sapphi-red commented 1 year ago

Ah, I noticed there's two issues around this one.

The first one is the duplication issue. This cannot be solved as I described https://github.com/vitejs/vite/issues/7504#issuecomment-1106675158.

The second one is the ordering issue. I noticed this can be solved by leaving the duplication issue. By including the output filename in hash calculation (or changing the meaning of filename argument of generateScopedName), I guess this can be solved. But this will be a breaking change so it won't be possible to include until next major.

In my case, I was looking at a compiled CSS output, which did not have duplicated styles. I assume this is because cssnano removes the duplicated styles at build time?

Yes, this should be because cssnano (or other css minifier) is removing the duplicated ones.

Am I wrong in thinking that by keeping only the first occurrence, the issue would be resolved for the most part?

This is not correct and will cause other issues (and maybe this ordering issue will also happen).

hyperscientist commented 1 year ago

I stumbled upon both these issues as soon as I implemented second route/page of the first app where I use these features. I am very surprised this issue isn't reported way more often.

For posterity: the solution to ordering issue is to change architecture of your css modules to never overwrite css properties declared on the base class. You should only build upon base class with new/unique properties but never overwrite. For complex cases you will have a tree of classes where only leaf nodes are actually used as classNames for HTML elements (you will also have plenty of mixins). Of course this only applies if you compose from other files - in the context of one file you can do whatever you want.

It feels to me that this is just a trait of CSS Modules in their current form and can not be fixed. To depend on ordering of CSS declarations feels fragile and prone to being mishandled by post processing plugins.

Tal500 commented 1 year ago

To dedupe .originalText, it will need to process all css files at once because it needs to know depency graph.

Is this hard to do with Vite plugins architecture? I think Vite can be very awesome in this field.

Merri commented 11 months ago

I think this issue should be fixable without processing all CSS files at once.

The problem here is that when a CSS Module is imported and it has a composes, Vite outputs all CSS to a single style element. This is the bug. Vite should output multiple style elements, one for each module. And, if a module of the same name has already been injected then simply do not output it again. After this there would be no more duplication of selectors. (Of course I'm no expert in Vite internals, behavior, nor limitations.)

I would like to see this fixed since I'm moving to Storybook 7 with Vite. This issue breaks our current code architecture. Many of our CSS Modules have zero specificity selectors in order to have no issues on consumer side of component library, avoiding potential issues with CSS file loading order. The existence of this bug forces me to increase specificity selectively based on whether a CSS Module uses composes or not. This will also force me to release a new major version of the library as increasing specificity is a potential breaking change. Also, working around this Vite composes issue will be extra mental load that nobody should be dealing with.

I think one of the reasons this issue does not come up that often is that you will mostly notice this issue if you are a veteran CSS Module user, or if you're moving existing code to use Vite. If you are less experienced with CSS Modules and/or are working on new code then you probably think CSS Modules is limited, and continue on.

For some humor to the ending, I will not be using Webpack for Storybook 7 because it will error with "out of memory" and debugging that would be worse than this issue in Vite... 😅

privatenumber commented 4 months ago

I've implemented a fix via https://github.com/privatenumber/vite-css-modules (which is getting integrated into Vite core via https://github.com/vitejs/vite/pull/16018).

Testing and feedback will be appreciated!

cssandstuff commented 4 months ago

This is awesome to get this fixed. If it's helpful to anyone else, I've previously overcome any ordering issues by making sure anything that is shared/composed is put in a css @layer{} so that you can then control how it will be stacked in the cascade and then get rid of any duplicates with css nano. (If you have the luxury of only supporting browsers that support css layers)

axelboc commented 4 months ago

Since browser support can be an issue with @layer, I've personally started documenting/implementing a dev guideline that forbids composing from another (React) component's CSS Module. If composition is needed, the base styles have to be moved to a "utility" CSS Module that must never be imported into a component:

/* utils.module.css (never imported directly) */
.baseBtn {
  color: blue;
}

/* OtherComponent.module.css (imported from `OtherComponent.jsx`) */
.otherBtn {
  composes: baseBtn from './utils.module.css';
}

/* ThisComponent.module.css (imported from `ThisComponent.jsx`) */
.thisBtn {
  /*composes: otherBtn from './OtherComponent.module.css';*/ /* FORBIDDEN */
  composes: baseBtn from './utils.module.css'; /* ALLOWED */
  color: red;
}

With @privatenumber's plugin, this guideline seems to remove any remaining ordering issue.

If multiple components import the same CSS module, you still get duplicated styles in development, but if multiple components' modules compose from the same utility module, this utility modules' styles are no longer duplicated and now always come before the components' modules.

The guideline could be enforced by a linting rule to avoid mistakes. It could be as simple as checking that the filename of the composed module starts with a lower case letter (since other linting rules enforce that React component files start with an uppercase letter).