-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Extract used CSS variables from .css
files
#17433
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
Conversation
crates/oxide/src/extractor/mod.rs
Outdated
@@ -139,6 +140,41 @@ impl<'a> Extractor<'a> { | |||
|
|||
extracted | |||
} | |||
|
|||
pub fn extract_css_variables_from_css_files(&mut self) -> Vec<Extracted<'a>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit of a mouthful, open to suggestions. Note: this is completely internal API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extract_variables_from_css
if you want a shorter name but this is fine imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that's nicer, and shorter, thanks!
This reverts commit 4ff21a3.
We will track CSS files while traversing the folder structure, but don't extract any normal candidates from these CSS files. We will also not include these files into any of the returned globs. We will just run the CSS extractor on these CSS files, and every time we find a CSS variable, we will verify whether it was used or not. For now, "using", just means if it is used inside of `var(…)`.
28392e8
to
481793b
Compare
@@ -402,6 +425,44 @@ fn read_all_files(changed_content: Vec<ChangedContent>) -> Vec<Vec<u8>> { | |||
.collect() | |||
} | |||
|
|||
#[tracing::instrument(skip_all)] | |||
fn extract_css_variables(blobs: Vec<Vec<u8>>) -> Vec<String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started passing options to the other parse_all_blobs
function but it looked a bit messy. So duplicated it instead (even though there is a good chunk of duplication going on).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the only difference really should be that we no-op the Candidate
machine though? Can you elaborate what you meant with messy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started passing through the contents and the extension and it leaked everywhere. Can see that in this commit: 7dc7878
Then I was thinking about an ExtractorOptions
struct that we could pass in as well, but that also required checking the options in the extract()
call at runtime. While that's not the end of the world, that's additional check we have to perform but it's a useless check for 99% of the files we scan. To make things a bit worse, the extract() function is called for every line in every file. So there could be a lot of unnecessary checks.
So instead of checking it in this hot path, I created a separate function instead.
It's shorter, and fits on a single line now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this was already merged but still left some questions
// Special handing for CSS files to extract CSS variables | ||
if extension == "css" { | ||
self.css_files.push(path); | ||
continue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary? Wouldn't we literally want to emit this in the files
list so we also ensure we have a watcher for it etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you are right I think. My thinking was that changing a CSS file triggers a full rebuild anyway. But that statement doesn't hold true for .module.css
files and they should be watched indeed.
We also have to be careful that we don't scan and watch the dist/out.css
file 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind following up on that? Just so we don't forget
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future reference. Follow up PR: #17467
@@ -139,6 +140,41 @@ impl<'a> Extractor<'a> { | |||
|
|||
extracted | |||
} | |||
|
|||
pub fn extract_variables_from_css(&mut self) -> Vec<Extracted<'a>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be done in a pre-processor where we already have logic to do that based on file extension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sort of but not really, we do have the contents and the extension so we do have all the information we need, but we return a new potentially transformed input file.
pub fn pre_process_input(content: &[u8], extension: &str) -> Vec<u8> { … }
But we don't emit extracted variables here. So we could update this logic to also emit Vec<Extracted>
.
Another solution is that we could transform the CSS file and replace everything that is not a used CSS variable with whitespace. That way the normal extractor won't find anything, and the CSS extractor will only find used variables.
But both these solutions seem like a hack, and a bit of an abuse for this pre_process_input
function 😅 unless you're thinking about something completely different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think this does make sense, agree that abusing pre-process for this does feel wrong too now that I think about it more.
@@ -402,6 +425,44 @@ fn read_all_files(changed_content: Vec<ChangedContent>) -> Vec<Vec<u8>> { | |||
.collect() | |||
} | |||
|
|||
#[tracing::instrument(skip_all)] | |||
fn extract_css_variables(blobs: Vec<Vec<u8>>) -> Vec<String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the only difference really should be that we no-op the Candidate
machine though? Can you elaborate what you meant with messy?
This PR is a follow-up PR for: #17433 In the other PR we allow scanning CSS files for extracting usages of CSS variables. This is important for `.module.css` files that reference these variables but aren't in the same big AST of the main CSS file. This PR also makes sure to watch for changes in those registered CSS files and re-extract the variables when they change. This PR took a bit longer than expected because I was trying to make sure that writing to `./dist/out.css` works without infinite-looping (e.g.: we had issues with this in Tailwind CSS v3 with webpack). But I couldn't reproduce the issue at all. I did had some code that tried to detect if the CSS file contained license headers and skip in (because then it's very likely an output CSS file) but even without it the tests were fine. I setup integration tests with `@tailwindcss/cli` itself, and with tools that use webpack. Added a test for Next.js, and a dedicated webpack test as well. Even without tests, locally, I couldn't reproduce an infinite loop due to changes in an output CSS file... Eventually dropped the code that tries to detect output CSS files. One thing to keep in mind is that if you change any of your "main" CSS files, then we will trigger a full rebuild anyway, so this change is only required for unrelated CSS files (like CSS module files) that use CSS variables. ## Test plan 1. Added integration tests for the CLI and Next.js 2. Added new dedicated test for webpack
This PR fixes an issue where CSS variables could be used in CSS modules, but where never emitted in your final CSS.
Some backstory, when Tailwind CSS v4 came out, we always emitted all CSS variables whether they were used or not.
Later, we added an optimization where we only emit the CSS variables that were actually used. The definition of "used" in this case is:
The issue this PR tries to solve is with the very first point. If you are using CSS modules, then every CSS file is processed separately. This is not a choice Tailwind CSS made, but how other build tooling works (like Vite for example).
To prevent emitting all of Tailwind's Preflight reset and all utilities per CSS file, you can use the
@reference
directive instead of repeating@import "tailwindcss";
. This is explained here: https://tailwindcss.com/docs/compatibility#explicit-context-sharingBut now we are just referencing them, not emitting them. And since the CSS module is not connected in any way to the main
index.css
file that contains the@import "tailwindcss";
directive, we don't even see the CSS variables while processing theindex.css
file. (or wherever your main CSS file is)This is where point 2 from above comes in. This is a situation where we rely on the extractor to find the used CSS variables so we can internally mark them as used.
To finally get to the point of this PR, the extractor only scans
.html
,.js
, ... files but not.css
files. So all the CSS variables used inside of CSS modules will not be generated.This PR changes that behavior to also scan
.css
files. But only for CSS variables (not any other type of class candidate). This is important, otherwise all your custom@utility foo {}
definitions would always markfoo
as a used class and include it in the CSS which is not always the case.On top extracting CSS variables, we will also make sure that the CSS variables we find are in usage positions (e.g.:
var(--color-red-500)
) and not in definition positions (e.g.:--color-red-500: #ff0000;
). This is important because we only want to emit the variables that are actually used in the final CSS output.One future improvement not implemented here, is that technically we will also extract CSS variables that might not be used if defined in a
@utility
.Fixes: #16904
Fixes: #17429
Test plan
.css
files (and ignored).css
files (and included)Testing on the reproduction defined in #16904, the
.module.css
file contains a reference tovar(--color-hot-pink)
, but generating a build shows that the variable definition is not available:When you run the build again with the changes from this PR, then we do see the definition of the

--color-hot-pink
in the root CSS file: