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

Migrate utilities in CSS files imported into layers #14617

Merged
merged 8 commits into from
Oct 10, 2024

Conversation

thecrypticace
Copy link
Contributor

@thecrypticace thecrypticace commented Oct 7, 2024

When a stylesheet is imported with @import “…” layer(utilities) that means that all classes in that stylesheet and any of its imported stylesheets become candidates for @utility conversion.

Doing this correctly requires us to place @utility rules into separate stylesheets (usually) and replicate the import tree without layers as @utility MUST be root-level. If a file consists of only utilities we won't create a separate file for it and instead place the @utility rules in the same stylesheet.

Been doing a LOT of pairing with @RobinMalfait on this one but I think this is finally ready to be looked at

@thecrypticace thecrypticace force-pushed the feat/codemod-stylesheet-graphs branch from 92f73ca to 1ec566f Compare October 7, 2024 18:47
Copy link
Member

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

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

This makes sense and is pretty straight-forward when reviewed by commits. Two things to consider though:

  1. Let's add a test for this scenario:

    --- ./a.css ---
    @import './c.css' layer(utilities);
    --- ./b.css ---
    @import './c.css';
    --- ./c.css ---
    .foo {
      color: red;
    }

    I would expect c.css to stills split into two files in this case with one not having the .foo class migrated to @utility or so. What do you think?

  2. We create new files now with [filename].utilities.css. Let's check if this already exited prior the migration though and how an error accordingly.

packages/@tailwindcss-upgrade/src/migrate.ts Outdated Show resolved Hide resolved
@RobinMalfait RobinMalfait force-pushed the feat/codemod-stylesheet-graphs branch from 1ec566f to 10f0a4c Compare October 8, 2024 13:43
@RobinMalfait RobinMalfait marked this pull request as draft October 8, 2024 20:16
@adamwathan
Copy link
Member

In v3 we never preserved the actual @layer rule for components and utilities, everything was just ultimately inlined no matter what, just in a specific location. Does that simplify anything?

Like in v3, this input:

// main.css
@import "my-crap.css" layer(utilities);

// my-crap.css
[potato] {
  background: brown;
}

...produced this output:

[potato] {
  background: brown;
}

...with no @layer utilities or anything present at all. So in terms of a codemod that preserves the same behavior shouldn't everything imported into a utilities or components layer be rendered at the top-level anyways?

@thecrypticace
Copy link
Contributor Author

@adamwathan Nah, I don't think it does. While v3 did produce that behavior we're still aiming to put things into the appropriate layer when migrating to v4 which means that [foo] should still be imported into a utility layer if it follows @import "tailwind/utilities" for example.

@thecrypticace thecrypticace marked this pull request as ready for review October 9, 2024 15:58
@thecrypticace thecrypticace force-pushed the feat/codemod-stylesheet-graphs branch 4 times, most recently from bc60358 to 15bea44 Compare October 9, 2024 17:36
@adamwathan
Copy link
Member

@adamwathan Nah, I don't think it does. While v3 did produce that behavior we're still aiming to put things into the appropriate layer when migrating to v4 which means that [foo] should still be imported into a utility layer if it follows @import "tailwind/utilities" for example.

Do you mean this?

@import "tailwindcss/utilities";

[foo] {
  background: red;
}

If so I don't think I agree, should remain unlayered, but also don't think that's what this PR is about anyways if I understand correctly so don't mean to derail.

I guess what I'm trying to think through in my head is what we would do if we were upgrading a project that had this in it by hand with a human touch:

/* main.css */
@import "my-components.css" layer(components);
@import "my-utilities.css" layer(utilities);

/* my-components.css */
.btn {
  background: blue;
  color: white;
  padding: 16px;
}

[potato] {
  background: brown;
}

/* my-utilities.css */
.tab-1 {
  tab-size: 1;
}

[foo] {
  background: red;
}

What would we have written instead if this was a v4 project?

@thecrypticace
Copy link
Contributor Author

Well… I would probably hoist all utilities to either the root or to their own file. But I think we also want to operate under principle of least surprise — if someone's placed their utilities in a deeper import tree then there's probably a structural reason they did that.

But — we could just collect them all and spit them out in their own file at the end. It would make some of this process much simpler.

Unfortunately we still have the multiple "roots" problem to deal with. If you import some into a utilities layer and some not then regardless the problem still exists for some paths to a specific file.

Aside: I wouldn't've placed utilities in imports like in the first place because I didn't even realize that worked until you pointed it out so I ultimately would never have run into this problem at all.

re the [foo] case:

We're talking about imported files only here but in any case how is this different from when we have to wrap imports?

We turn this:

@import "tailwindcss/utilities";
@import "./foo.css";

into this (was already the case before this PR):

@import "tailwindcss/utilities";
@import "./foo.css" layer(utilities);

which, if inlined, is the same as this:

@import "tailwindcss/utilities";
[foo] {
  background: red;
}

becoming this — which I think we also do cc @RobinMalfait:

@import "tailwindcss/utilities";
@layer utilities {
  [foo] {
    background: red;
  }
}

@RobinMalfait
Copy link
Member

@import "tailwindcss/utilities";
[foo] {
  background: red;
}

This would stay as-is (we don't wrap the rules after the last tailwind directive (or import) into an @layer). But I think we still do add the layer(utilities) after the last tailwind directive.

So I think this is still true:

Input:

@import "tailwindcss/utilities";
@import "./foo.css";

Output:

@import "tailwindcss/utilities" layer(utilities);
@import "./foo.css" layer(utilities);

thecrypticace and others added 8 commits October 10, 2024 09:11
Sometimes a node may not have a `before` raws prop
Here we introduce a new  `dumpFiles` helper that can be used to list all files matching a given glob and their content. This simplifies some assertions and ensures files are not accidentally added or deleted.
The purpose of the `Stylesheet` class is to collect and hold relevant information about CSS files that can be used for more advanced transformations.

Additionally this unlocks two things:
- Formatting only happens after all transformations have completed
- Files are only written once at the end — which will simplify the introduction of a `--dry-run` option in the future
CSS files may be imported and we’ll need to know what files import other files in order to unlock more advanced transformations
When a stylesheet is imported with `@import “…” layer(utilities)` that means that all classes in that stylesheet and any of its imported stylesheets become candidates for `@utility` conversion
Now that we convert layered, imported rules into `@utility` where needed we need to hoist these rules outside of their imported layer since `@utility` MUST be root-level.
@thecrypticace thecrypticace force-pushed the feat/codemod-stylesheet-graphs branch from 15bea44 to 589d0e8 Compare October 10, 2024 13:18
@RobinMalfait RobinMalfait enabled auto-merge (squash) October 10, 2024 13:39
@RobinMalfait RobinMalfait merged commit 4d1becd into next Oct 10, 2024
1 check passed
@RobinMalfait RobinMalfait deleted the feat/codemod-stylesheet-graphs branch October 10, 2024 13:44
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.

4 participants