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

Re-enable: Only expose used CSS variables #16676

Merged
merged 8 commits into from
Feb 21, 2025

Conversation

philipp-spiess
Copy link
Member

@philipp-spiess philipp-spiess commented Feb 19, 2025

This PR re-enables the changes necessary to remove unused theme variables and keyframes form your CSS.

This change was initially landed as #16211 and then later reverted in #16403 because we found some unexpected interactions with using @apply and CSS variables in multi-root setups like CSS modules or Vue inline <style> blocks that were no longer seeing their required variables defined.

This issue is fixed by now ensuring that theme variables that are defined within an @reference "…" boundary will still be emitted in the generated CSS when used (as this would otherwise not generate a valid stylesheet).

So given the following input CSS:

@reference "tailwindcss";
.text-red {
  @apply text-red-500;
}

We will now compile this to:

@layer theme {
  :root, :host {
    --text-red-500: oklch(0.637 0.237 25.331);
  }
}
.text-red {
  color: var(--text-red-500);
}

This PR also improves the initial implementation to not mark theme variables as used if they are only used to define other theme variables. For example:

@theme {
  --font-sans:
    ui-sans-serif, system-ui, sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji', 'Segoe UI Symbol',
    'Noto Color Emoji';
  --font-mono:
    ui-monospace, SFMono-Regular, Menlo, Monaco, Consolas, 'Liberation Mono', 'Courier New',
    monospace;

  --default-font-family: var(--font-sans);
  --default-mono-font-family: var(--font-mono);
}

.default-font-family {
  font-family: var(--default-font-family);
}

This would be reduced to the following now as --font-mono is only used to define another variable and never used outside the theme block:

:root, :host {
  --font-sans:
    ui-sans-serif, system-ui, sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji', 'Segoe UI Symbol',
    'Noto Color Emoji';
  --default-font-family: var(--font-sans);
}

.default-font-family {
  font-family: var(--default-font-family);
}

Test plan

  • See updated unit and integration tests
  • Validated it works end-to-end by using a SvelteKit example

@philipp-spiess philipp-spiess force-pushed the feat/cleanup-unused-vars-v2 branch 2 times, most recently from c772a81 to b3cb5c3 Compare February 19, 2025 23:03
@philipp-spiess philipp-spiess marked this pull request as ready for review February 20, 2025 14:23
@philipp-spiess philipp-spiess requested a review from a team as a code owner February 20, 2025 14:23
@philipp-spiess philipp-spiess force-pushed the feat/cleanup-unused-vars-v2 branch from b3cb5c3 to 0dce342 Compare February 20, 2025 14:28
@philipp-spiess philipp-spiess merged commit 7bece4d into main Feb 21, 2025
5 checks passed
@philipp-spiess philipp-spiess deleted the feat/cleanup-unused-vars-v2 branch February 21, 2025 09:45
@moatorres
Copy link

moatorres commented Feb 21, 2025

Great work, @philipp-spiess! Am I right to assume that this will be included on v4.0.8?

@philipp-spiess
Copy link
Member Author

@moatorres Yep. We're preparing the release right now so hopefully it's out in a couple of hours 👍

@dpi
Copy link

dpi commented Feb 21, 2025

Thanks @philipp-spiess !

@doits
Copy link

doits commented Feb 21, 2025

Sorry for hijacking this, but how can I force to output some of the default theme variables even if they are not detected automatically? E.g. how can I always emit all the--color-* variables (because I use them in other places dynamically where the values come from the database and cannot be inferred just by scanning the code)?

@wenbei
Copy link

wenbei commented Feb 21, 2025

@doits You can use the new static keyword, see the original #16211

@gene-git
Copy link

Should 4.0.8 have fixed #16725 ?

@wongjn
Copy link
Contributor

wongjn commented Feb 21, 2025

No, I think this PR is what is causing #16725.

@gene-git
Copy link

Thanks @wongjn - makes more sense :)

I downgraded to 4.0.7 but problem remains. I will try going back to 4.0.6

@gene-git
Copy link

4.0.6 has same problem. 4.0.6 with postcss rolled back to 8.5.2 same problem as well.

Now I am perplexed. This should be same as production server is running.

Suggestions?

@wongjn
Copy link
Contributor

wongjn commented Feb 21, 2025

Try checking you have successfully downgraded, via npm ls tailwind - you may have different versions installed as deeper dependencies.

@gene-git
Copy link

gene-git commented Feb 21, 2025

you're correct - 4.0.8 is pulled in and visible in package-lock.json.

I deleted node_modules and .next and package-lock.json and package.json has 4.0.6

grep tailwindcss package.json
    "@tailwindcss/typography": "^0.5.16",
    "@tailwindcss/postcss": "^4.0.6",
    "tailwindcss": "^4.0.6"

Yet after doing npm install it shows:

$ npm ls tailwindcss

├─┬ @tailwindcss/postcss@4.0.8
│ ├─┬ @tailwindcss/node@4.0.8
│ │ └── tailwindcss@4.0.8 deduped
│ └── tailwindcss@4.0.8 deduped
├─┬ @tailwindcss/typography@0.5.16
│ └── tailwindcss@4.0.8 deduped
└── tailwindcss@4.0.8

Hmm. My ignorance clearly showing - what is the correct way to get 4.0.6 ?

Okay this seems to work:

npm install @tailwindcss/postcss@4.0.6 @tailwindcss/node@4.0.6 tailwindcss@4.0.6

Now its back to 4.0.6 and builds again.

Thank you for your help @wongjn.

@wongjn
Copy link
Contributor

wongjn commented Feb 21, 2025

If you're using git, you could revert package.json and package-lock.json to a commit before 4.0.8 was installed. Otherwise, you could force 4.0.7 or 4.0.6 like:

npm install @tailwindcss/postcss@4.0.6 tailwindcss@4.0.6

@gene-git
Copy link

Thanks @wongjn - yep I'm using git so for sure I can go back to the production tag.

@gene-git
Copy link

Does anyone have thoughts on how to fix #16725 ?

@doits
Copy link

doits commented Feb 21, 2025

@doits You can use the new static keyword, see the original #16211

@wenbei thanks for the tip, I tried it but got stuck at how to name the variable to include all colors, e.g. tried this:

@theme static {
  --color-*: <XXX>;
}

with <XXX> as static, default, initial and inherit (my four best guesses) – it failed to compile with each, for example:

Error: Invalid theme value static for namespace --color-*

Or must I copy all the variables from the docs and put them there explicitly? I guess this works in the meantime but then I won't get any update if the theme is changed 🤔

@doits
Copy link

doits commented Feb 21, 2025

... just found out in #16514 how to include all variables:

@import "tailwindcss" theme(static);

Works for my use case.

Though I'd still be interested if you can make only some of the variables static (e.g. only all --color-* ones).

philipp-spiess added a commit that referenced this pull request Feb 25, 2025
…larations (#16774)

Fixes #16725

When using `@reference "tailwindcss";` inside a separate CSS root (e.g.
Svelte `<style>` components, CSS modules, etc.), we have no guarantee
that the CSS variables will be defined in the main stylesheet (or if
there even is one). To work around potential issues with this we decided
in #16676 that we would emit all used CSS variables from the `@theme`
inside the `@reference` block.

However, this is not only a bit surprising but also unexpected in CSS
modules and Next.js that **requires CSS module files to only create
scope-able declarations**. To fix this issue, we decided to not emit CSS
variables but instead ensure all `var(…)` calls we create for theme
values in reference mode will simply have their fallback value added.

This ensures styles work as-expected even if the root Tailwind file does
not pick up the variable as being used or _if you don't add a root at
all_. Furthermore we do not duplicate any variable declarations across
your stylesheets and you still have the ability to change variables at
runtime.

## Test plan

- Updated snapshots everywhere (see diff)
- New Next.js CSS modules integration test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants