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

CSS Modules - composed styles are duplicated #7504

Open
7 tasks done
desmond-tuiyot opened this issue Mar 28, 2022 · 16 comments · May be fixed by #16018
Open
7 tasks done

CSS Modules - composed styles are duplicated #7504

desmond-tuiyot opened this issue Mar 28, 2022 · 16 comments · May be fixed by #16018
Labels
feat: css has workaround p3-minor-bug An edge case that only affects very specific usage (priority)

Comments

@desmond-tuiyot
Copy link

desmond-tuiyot commented Mar 28, 2022

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
Copy link
Member

ygj6 commented Mar 30, 2022

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
Copy link
Author

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
Copy link
Member

ygj6 commented Mar 31, 2022

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

@ygj6 ygj6 added p3-minor-bug An edge case that only affects very specific usage (priority) and removed pending triage labels Mar 31, 2022
@xiaoxian521
Copy link

I also have the same problem

@fi3ework
Copy link
Member

fi3ework commented Apr 6, 2022

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
Copy link
Author

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
Copy link
Member

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

  • original.module.css will include .originalText
  • small.module.css will include .originalText and .smallText

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
Copy link
Member

ygj6 commented Apr 27, 2022

FYI, madyankin/postcss-modules#32

@axelboc
Copy link

axelboc commented Aug 10, 2022

In #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
Copy link
Member

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

The first one is the duplication issue. This cannot be solved as I described #7504 (comment).

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
Copy link

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
Copy link
Contributor

Tal500 commented Oct 10, 2022

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
Copy link

Merri commented Jul 31, 2023

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
Copy link
Contributor

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

Testing and feedback will be appreciated!

@cssandstuff
Copy link

cssandstuff commented Mar 18, 2024

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
Copy link

axelboc commented Mar 18, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: css has workaround p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.