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

Ensure @config is injected in common ancestor sheet #14989

Merged
merged 7 commits into from
Nov 14, 2024

Conversation

RobinMalfait
Copy link
Member

@RobinMalfait RobinMalfait commented Nov 13, 2024

This PR fixes an issue where an @config was injected in a strange location if you have multiple CSS files with Tailwind directives.

Let's say you have this setup:

/* ./src/index.css */
@import "./tailwind-setup.css";

/* ./src/tailwind-setup.css */
@import "./base.css";
@import "./components.css";
@import "./utilities.css";

/* ./src/base.css */
@tailwind base;

/* ./src/components.css */
@tailwind components;

/* ./src/utilities.css */
@tailwind utilities;

In this case, base.css, components.css, and utilities.css are all considered Tailwind roots because they contain Tailwind directives or imports.

Since there are multiple roots, the nearest common ancestor should become the tailwind root (where @config is injected). In this case, the nearest common ancestor is tailwind-setup.css (not index.css because that's further away).

Before this change, we find the common ancestor between base.css and components.css which would be index.css instead of tailwind-setup.css.

In a next iteration, we compare index.css with utilities.css and find that there is no common ancestor (because the index.css file has no parents). This resulted in the @config being injected in index.css and in utilities.css.

Continuing with the rest of the migrations, we migrate the index.css's @config away, but we didn't migrate the @config from utilities.css.

With this PR, we don't even have the @config in the utilities.css file anymore.

Test plan

  1. Added an integration test with a non-migrateable config file to ensure that the @config is injected in the correct file.
  2. Added an integration test with a migrateable config file to ensure that the CSS config is injected in the correct file. h/t @philipp-spiess
  3. Ran the upgrade on the https://commit.tailwindui.com project and ensured that
  4. The @config does not exist in the utilities.css file (this was the first bug we solved)
  5. The @config is replaced in the tailwind.css file correctly.
image image

@RobinMalfait RobinMalfait requested a review from a team as a code owner November 13, 2024 15:14
@RobinMalfait RobinMalfait force-pushed the fix/incorrect-config-link branch 2 times, most recently from 9491340 to 207183b Compare November 13, 2024 15:22
Comment on lines +1484 to +1488
plugins: [
() => {
// custom stuff which is too complicated to migrate to CSS
},
],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
plugins: [
() => {
// custom stuff which is too complicated to migrate to CSS
},
],
theme: {
extend: {
colors: {
'my-red': 'red',
},
},
},

When we update the config to something that we can convert to, we noticed that the @theme now ends up in src/index.css and that the @config is still inside src/tailwind-setup.css.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a failing test for this: a8a66a4

Then fixed it: dc5cd8d

Bonus points: basically deleted most of the code in the migrate-config file

Once we found a common parent between two sheets, we can stop finding
the parent and continue to the next sheets.
@RobinMalfait RobinMalfait force-pushed the fix/incorrect-config-link branch from 207183b to 36bceb0 Compare November 13, 2024 17:23
We already computed the correct Tailwind root file (and linked the JS
config to it). This means that we can remove the logic in the
migrate-config migration to find the correct location.
@RobinMalfait RobinMalfait merged commit 8538ad8 into next Nov 14, 2024
1 check passed
@RobinMalfait RobinMalfait deleted the fix/incorrect-config-link branch November 14, 2024 10:48
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.

3 participants