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(css): treeshake css modules #16051

Merged
merged 1 commit into from Mar 12, 2024
Merged

fix(css): treeshake css modules #16051

merged 1 commit into from Mar 12, 2024

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Feb 28, 2024

Description

ref: #4389

I'm not sure if it's a big oversight, but we're able to treeshake CSS modules as long as we mark it side-effect-free. Because if the exported values were not used, it's also safe to remove the CSS completely.

Additional context

The only small breaking change is that if someone relied on :global classes to be included today, but this PR will treeshake it away. I think they should place it as a global CSS instead, so probably a non-issue (?)


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.
  • Ideally, include relevant tests that fail without this PR but pass with it.

@bluwy bluwy added feat: css p3-significant High priority enhancement (priority) labels Feb 28, 2024
Copy link

stackblitz bot commented Feb 28, 2024

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

Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

It'd be great if we can detect whether :global is used and turn it off. But I don't find a way.

IIUC the behavior of the following case would change, but I think it's fine.

:global(.foo) {
  color: red;
}

:global(.bar) {
  color: green;
}
.baz {
  color: blue;
}
import { baz } from './foo.module.css'

// doesn't use baz

@sapphi-red
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci

This comment was marked as outdated.

@bluwy
Copy link
Member Author

bluwy commented Feb 29, 2024

It'd be great if we can detect whether :global is used and turn it off. But I don't find a way.

Yeah. I think if they wrote it that way, the intent was probably also that if the CSS is not used, then the global CSS would also be not included. Still since we're changing it now they'll be a small risk.

I also re-ran the vite-plugin-react-pages CI, not really sure what's failing there.

@vite-ecosystem-ci
Copy link

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

Nice one! Maybe we should merge this in Vite 5.2 just in case, but ok with doing it in a patch if you think it is safe enough.

@bluwy
Copy link
Member Author

bluwy commented Feb 29, 2024

Yeah I think we can merge it for 5.2. Not in a rush to get this out, mainly eyeing on #16058 instead 😄

@bluwy bluwy added this to the 5.2 milestone Feb 29, 2024
@bluwy bluwy merged commit 17d71ec into main Mar 12, 2024
10 checks passed
@bluwy bluwy deleted the treeshake-css-modules branch March 12, 2024 13:13
@zhengweipx
Copy link

The global CSS of the package will be Shaked, but I need these CSS. How should I handle them?
layout.module.less -> :global { .baz { background: red; } }
import "@/assets/styles/layout.module.less";

@bluwy
Copy link
Member Author

bluwy commented Apr 8, 2024

You can use layout.less instead? Is there a reason to use .module.less that only contains global styles or not use it's exported properties?

@zhengweipx
Copy link

You can use layout.less instead? Is there a reason to use .module.less that only contains global styles or not use it's exported properties?

Yes, I can use layout.less instead. There is no reason to use .module.less that only contains global styles if you are not using its exported properties.

@tobi-fis
Copy link

tobi-fis commented Apr 9, 2024

I've got a bunch of :root CSS variables in a file called vars.module.css. This, along with other CSS module files, is imported in my main.module.css, which is in turn is imported in the main.ts of my Vue application, aggregating all the CSS I need globally. So I am providing reused classes and variables down to all components as I've always done.

But since updating vite to >5.2.0, all :root CSS variables imported via the main.module.css are missing in the build of my application. I am not familiar with the internals of treeshaking, but I believe this fix right here is the culprit for my problem.

Is this a bug, or actually desired behaviour? And what can I do to fix it, apart from downgrading to vite 5.1.7 again?
The only workaround I've found is importing the main.module.css in the <style> tag of my root vue component (e.g. App.vue)

Thank you.

Edit: It seems that all CSS vars declared in the :root scope are removed from build, even when defined in a module.css file alongside class definitions (which do not get treeshaken). I also tried removing the .module from the vars.css, with no difference in the outcome.

Edit 2: I noticed that removing the .module from vars.css and importing it directly in my main.ts does not cause the vars to disappear.
So the issue here only appears when importing :root CSS variables via a .module.css (directly or indirectly) file in the main.ts of a Vue application.

@bluwy
Copy link
Member Author

bluwy commented Apr 10, 2024

@tobi-fis If the :root exists in main.module.css, and you are using main.module.css like: import style from './main.module.css', and the :root is gone after build, that sounds like a bug to me, but I'm not sure how this PR would affect that.

What this PR does is that if main.module.css was never used, e.g. if you simply do import './main.module.css' without importing anything from it, then it would get treeshaken away. Because CSS modules contains and exports scoped classes but it's not being used.

If you're seeing the case in the first paragaph, I'd appreciate if you can open a new issue with a repro.

@tobi-fis
Copy link

@bluwy
Thanks for your reply!

It is actually the second case you described. So I guess this is desired behaviour now and we will have to adapt to it.

@TuureKaunisto
Copy link

TuureKaunisto commented Apr 24, 2024

Is there a configuration option to disable css tree-shaking?

We're facing the breaking change mentioned in the original comment: We have some components that have all their styles within a :global { } block. As a workaround we've added and imported a single class with a redundant declaration on those files so they won't get excluded by tree-shaking.

Here's an example of the suboptimal workaround:

tooltip.module.scss:

.tooltip {
  position: relative;
}

:global {
  .tooltip {
    position: relative;
    ...
  }
}

tooltip.tsx:

import style from './tooltip.module.scss'
...
return <div className={`${style.tooltip} tooltip`}> ...

@bluwy
Copy link
Member Author

bluwy commented Apr 25, 2024

You can use this Vite plugin if you want to workaround it for now:

function cssModuleSideEffectful() {
  return {
    name: 'css-module-side-effectful',
    enforce: 'post'
    transform(_, id) {
      if (id.includes('.module.')) {
        return { 
          moduleSideEffects: 'no-treeshake' // or true, which i think also works with slightly better treeshake
        }
      }
    }
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: css p3-significant High priority enhancement (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants