Only expose used CSS variables #16211
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RobinMalfait
force-pushed
the
feat/only-expose-used-variables
branch
5 times, most recently
from
0f19b62 to
8f74b3e
Compare
crates/oxide/src/parser.rs
Outdated
Comment on lines
311
to
316
| if candidate.starts_with(b"--") { | ||
| match input.get(start_idx - 4..start_idx) { | ||
| Some(b"var(") => return ValidationResult::Valid, | ||
| _ => return ValidationResult::Invalid, | ||
| } | ||
| } |
There was a problem hiding this comment.
I think conceptually it would be better to not mix the two concept of candidates and variables names in the parsing code since it's already quite convoluted here... I wonder if this is the time where we go back to the drawing board and try to make this a bit easier to understand?
There was a problem hiding this comment.
I think that this PR could be a stepping stone to a new/better implementation but I feel like this PR should not be blocked by that, especially since it's all internal/private API anyway.
This will also have an influence on the build(candidates) in core, which is also something we could tackle.
Both should be new PRs imo.
crates/oxide/src/parser.rs
Outdated
| // CSS variables must be preceded by `var(` to be considered a valid CSS variable candidate | ||
| if candidate.starts_with(b"--") { | ||
| match input.get(start_idx - 4..start_idx) { | ||
| Some(b"var(") => return ValidationResult::Valid, |
There was a problem hiding this comment.
Also getPropertyValue, theme(, and --theme( at the very least. We probably should just track everything that starts with -- so that this also works:
let var = '--my-var';
let style = bla.getPropertyValue(var);There was a problem hiding this comment.
theme(and--theme(are not required because they may or may not use the variables internally so we will see them in the AST already.- If we want to make the
getPropertyValuealso work, then we actually don't need to do anything because we already discovered these strings as candidates.
RobinMalfait
force-pushed
the
feat/only-expose-used-variables
branch
from
c0cc98c to
0b58b52
Compare
RobinMalfait
added a commit
that referenced
this pull request
Feb 5, 2025
This PR introduces a performance improvement we noticed while working on on: #16211 We noticed that `substituteFunctions` was being called on every `node` after the `compileAstNodes` was done. However, the `compileAstNodes` is heavily cached. By moving the `substituteFunctions` we into the cached `compileAstNodes` we sped up performance for Catalyst rebuilds from ~15ms to ~10ms. | Before | After | | --- | --- | | <img width="710" alt="image" src="https://github.com/user-attachments/assets/eaf110d9-2f88-447c-9b10-c77d47bd99a5" /> | <img width="696" alt="image" src="https://github.com/user-attachments/assets/c5a2ff4c-d75e-4e35-a2b6-d896598810f5" /> |
RobinMalfait
force-pushed
the
feat/only-expose-used-variables
branch
from
0b58b52 to
ac2a0c9
Compare
This currently only marks it as `used`, _if_ it's resolved via the `#var(…)` theme method.
We can only mark them as being used if the utility or variant itself is being used.
If only existing variables have been found, then there is nothing to do.
This has some retries built-in
Instead of trying to traverse ASTs and tracking unused variables in
various spots, let's reverse the algorithm.
ALWAYS emit everything, this has the added benefit that emitting CSS
variables and keyframes is only done once (in the `compile(…)` step, not
in each `build(…)` step).
However, the removal of the unused CSS variables is part of the
`optimizeAst` step. This has a few benefits:
1. All the logic is done in a single spot, instead of tracking all over
the codebase.
2. We are already traversing for the optimization step, so we can hook
in without additional walks of the entire AST.
3. It will be more correct, e.g.: if you have a valid utility, but
invalid variant, since the utility is handled first it could be that
we mark variables as used even though the no CSS will be generated
due to the invalid variant.
Since this the `optimizeAst` step never even sees the thrown-away
variant+utility, it means that we don't even have to worry about
this.
Had to add a context node, to different between `:root {}` coming from
`@theme` or coming from user CSS (which should stay untouched).
Before this, `--width-1\/2` would be handled as:
```json
[
{ kind: 'word', value: '--width-\\' }
{ kind: 'separator', value: '/' }
{ kind: 'word', value: '2' }
]
```
But now it will be handled as:
```json
[
{ kind: 'word', value: '--width-\\/2' }
]
```
This makes sure that the `optimizeAst` goes from ~11ms back to ~5ms
This was already the case before, but we were also verifying that it was preceded by `var(`. Let's remove that code to make sure code like this works: ```js let var = '--my-var'; let style = bla.getPropertyValue(var); ```
RobinMalfait
force-pushed
the
feat/only-expose-used-variables
branch
from
ac2a0c9 to
cac548c
Compare
This will make sure that all variable are emitted regardless of whether it's used or not.
thecrypticace
approved these changes
Feb 6, 2025
thecrypticace
added a commit
to tailwindlabs/tailwindcss-intellisense
that referenced
this pull request
Feb 6, 2025
IntelliSense part of tailwindlabs/tailwindcss#16211
|
Hi, I'm very glad to see this is being worked on. I asked about this early May last year, but at the time this was pushed for "possible for future" which was understandable. Question regarding the implementation: does this only apply to variables in the "tailwindcss/theme" blocks? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR only exposes used CSS variables.
My initial approach was to track the used variables, this was a bit messy because it meant that we had to walk part of the AST(s) in multiple places. We also had to be careful because sometimes if a variable exists in an AST, that doesn't mean that it's actually used. E.g.:
In this last case, the
--color-blue-500is part of the CSS AST, but as long asfoothe utility is not used, it won't end up in your actual CSS file, therefore the variable is not used.Alternatively, if the
fooutility is used with an invalid variant (e.g.:group-[>.foo]:foo, then the@utility foocode will still run internally because variants are applied on top of the utility. This means that it looks likevar(--color-blue-500)is being used.Another annoying side effect was that because variables are conditionally generated, that the
@theme->:root, :hostconversion had to happen for every build, instead of once in thecompile(…)step.To prevent all the messy rules and additional booking while walking of ASTs I thought about a different approach. We are only interested in variables that are actually used. The only way we know for sure, is right before the
toCss(…)step. Any step before that could still throw away AST nodes.However, we do have an
optimizeAststep right before printing to simplify and optimize the AST. So the idea was to keep all the CSS variables in the AST, and only in theoptimizeAststep we perform a kind of mark-and-sweep algorithm where we can first check which variables are actually used (these are the ones that are left in the AST), and later we removed the ones that weren't part of known used list.Moving the logic to this step feels a natural spot for this to happen, because we are in fact optimizing the AST. We were already walking the AST, so we can just handle these cases while we are walking without additional walks. Last but not least, this also means that there is only a single spot where need to track and remove variables.
Now, there is a different part to this story. If you use a variable in JS land for example, we also want to make sure that we keep the CSS variable in the CSS. To do this, we can mark variables as being used in the internal
Theme.The Oxide scanner will also emit used variables that it can find such as
var(--color-red-500)and will emit--color-red-500as a "candidate". We can then proactively mark this one as used even though it may not be used anyway in the actual AST.Always including all variables
Some users might make heavy use of JavaScript and string interpolation where they need all the variables to be present. Similar to the
inlineandreferencetheme options, this also exposes a newstaticoption. This ensures that all the CSS variables will always be generated regardless of whether it's used or not.One handy feature is that you have granular control over this:
Performance considerations:
Now that we are tracking which variables are being used, it means that we will produce a smaller CSS file, but we are also doing more work (the mark-and-sweep part). That said, ran some benchmarks and the changes look like this:
Running it on Catalyst:
(probably within margin of error)
Running it on Tailwind UI:
Test plan
The diff on Catalyst looks like this:
If you have
ripgrepinstalled, you can use this command to verify that these variables are indeed not used anywhere:The only exception I noticed is that we have this:
But this is not a variable, but it's replaced at build time with the actual value, so this is not a real issue.
Testing on other templates:
Fixes: #16145