-
-
Notifications
You must be signed in to change notification settings - Fork 504
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
feat(linter): implement class sorting rule (first pass) #1362
Conversation
✅ Deploy Preview for biomejs canceled.
|
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.
Thank you @DaniGuardiola for kicking off this!
Here's some preliminary feedback, please take it with a grain of salt as I have little knowledge of the tool tailwind, and it's still unclear how you want to implement this feature gradually:
- I'd remove all the options for now, it make the code unclear and we can do it later
- I would start moving functions and logic in a separate file or multiple files. While doing that, I would appreciate if we start adding tests. Not just integration tests, but unit tests too. In Rust is very easy to create unit tests.
So please do that so we can also see how the logic should work
// some_file.rs fn class_parser(&str) -> Result<SomeData, String> { } #[cfg(test)] mod tests { use super::class_parser; #t[test] fn should_parse_something() { let result = class_parser(""); expect(result.is_ok()); } }
- Document stuff. This is really important in Biome. We want to make sure that the inner logic of our code is documented and understandable by any contributor. We believe this helps maintainability and PR reviews
- There's a lot of string allocations, but it's fine for now. We can deal with that later once we progress with the developments
- I understood that there's a lot of work to do, and I believe we can gradually add features in separate PRs. So, I think it's best to make a plan for this PR, review it, merge and continue with more work. You can also create new draft PRs based on this one so you feel stuck in case our reviews are slow to come
What do you think? Again, great work and thank you!
crates/biome_js_analyze/src/semantic_analyzers/nursery/use_sorted_classes.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/semantic_analyzers/nursery/use_sorted_classes.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/semantic_analyzers/nursery/use_sorted_classes.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/semantic_analyzers/nursery/use_sorted_classes.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/semantic_analyzers/nursery/use_sorted_classes.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/semantic_analyzers/nursery/use_sorted_classes.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/semantic_analyzers/nursery/use_sorted_classes.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/semantic_analyzers/nursery/use_sorted_classes.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/semantic_analyzers/nursery/use_sorted_classes.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/semantic_analyzers/nursery/use_sorted_classes.rs
Outdated
Show resolved
Hide resolved
Thanks for the review @ematipico! I agree that it might be a good idea to groom and consolidate the current code before adding more features. I'm gonna focus on that so we can merge this and keep working in separate PRs. I would only highlight that the sorting algorithm is very complex, and I have iterated a lot, so the code is a bit frankensteiny at the moment with a mixture of obsolete POC stuff and "big-picture" decisions. My goal is going to be to get this in a good state and in line with the "big-picture" approach consistently (without adding features or unnecessary complexity). Sound good? Responding to your preliminary feedback:
I think removing most options is fine, and I can just check this branch's history to re-introduce them later (a decent amount of work went into them, so I'd like to re-use them in the future). I would like to preserve two options though, because they are super-simple and central to the rule: For the rest, we can just use the tailwind preset directly for now.
Yep, I'd love to do that. Still new to Rust and the module system has me a bit confused, so I'd appreciate any guidance on this. Re:tests, of course, I was just focusing on getting things right first but I think some things can be tested now, like the class parser.
100%. As with tests, I was waiting for things to stabilize, and for the big picture to emerge.
Yes, this was jarring to me too. I even asked about it in the Discord. As a Rust (and Biome) beginner, I was a bit helpless here, partly because the presets are supposed to be in the same format as options, but string slices can't be deserialized apparently. Any help would be super appreciated, I wanna make this rule as fast as possible!
Agreed! Let's do that. I'm gonna focus on that: address your comments, groom it, get it in a good state, and merge. Then I can keep working on features. In parallel, I'm thinking about writing a deep dive in my post about how Tailwind sorting works, so that there's something I can reference. Thanks again! PD: after I go offline today, I'll be afk for a couple of days. Be back when the new years hangover is over :P |
@ematipico I did the following:
Before merging, I still wanna do a full documentation pass and address some of the TODOs. When you have a moment, please re-review :) happy new year! |
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.
Hey @DaniGuardiola
Thank you! I pushed some code in the branch. Let's:
- do one more pass of documentation if this is still needed
- add tests. We can't merge the rule without integration tests
crates/biome_js_analyze/src/semantic_analyzers/nursery/use_sorted_classes.rs
Outdated
Show resolved
Hide resolved
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 made a quick pass and left several comments :)
crates/biome_js_analyze/src/semantic_analyzers/nursery/use_sorted_classes.rs
Show resolved
Hide resolved
crates/biome_js_analyze/src/semantic_analyzers/nursery/use_sorted_classes.rs
Show resolved
Hide resolved
crates/biome_js_analyze/src/semantic_analyzers/nursery/use_sorted_classes.rs
Outdated
Show resolved
Hide resolved
.../biome_js_analyze/src/semantic_analyzers/nursery/use_sorted_classes/any_class_string_like.rs
Show resolved
Hide resolved
.../biome_js_analyze/src/semantic_analyzers/nursery/use_sorted_classes/any_class_string_like.rs
Outdated
Show resolved
Hide resolved
.../biome_js_analyze/src/semantic_analyzers/nursery/use_sorted_classes/any_class_string_like.rs
Outdated
Show resolved
Hide resolved
.../biome_js_analyze/src/semantic_analyzers/nursery/use_sorted_classes/any_class_string_like.rs
Outdated
Show resolved
Hide resolved
.../biome_js_analyze/src/semantic_analyzers/nursery/use_sorted_classes/any_class_string_like.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/semantic_analyzers/nursery/use_sorted_classes/options.rs
Outdated
Show resolved
Hide resolved
Thank you very much, @Conaclos! I responded to all of your comments and will take action on them soon. |
@DaniGuardiola Fell free to resolve the reviews once addressed. |
7ba207b
to
38c964d
Compare
38c964d
to
f74aec0
Compare
crates/biome_js_analyze/src/semantic_analyzers/nursery/use_sorted_classes.rs
Show resolved
Hide resolved
How do I get this to run on save? I tried these settings without any luck "linter": {
"rules": {
"nursery": {
"useSortedClasses": "error" "editor.codeActionsOnSave": {
"quickfix.biome": "explicit",
"source.fixAll": "explicit"
}, |
You can't. The code action is unsafe. You can only opt-in via editor. Or via |
For anyone interested, I found this workaround (automatic class sorting is a must!): Install the extension Add this to your config:
Thank you for building this! |
Ah just using pnpm biome lint directly, Thank you!
|
neither
hope this helps someone :) |
If you want it to work on explicit save you can go to your vscode's keybindings.json and add this:
|
When do you think this option will cease to be “unsafe”? |
@devcaeg you can also configure it now:
|
The problem with this is that in the autosave the “useSortedClasses” fix is not applied. |
I take back what was said before, with the following configuration in "editor.codeActionsOnSave": {
"quickfix.biome": "explicit"
} |
For additional clarity on @luckydye's comment, here's where to insert the {
"$schema": "https://biomejs.dev/schemas/1.9.4/schema.json",
...
"linter": {
...
"rules": {
...
"nursery": {
"useSortedClasses": {
"level": "error",
"fix": "safe",
"options": {
"attributes": ["className"],
"functions": ["cn"]
}
}
}
}
}
} |
Related: #1274
Summary
A class sorting rule for Tailwind CSS classes and other tools involving utility classes / atomic classes (initial version).
Test Plan
Unit tests and integration tests.
How it works
The pipeline is this:
Some examples for clarity:
"sort config"
ordered layers + index in each layer, e.g:
index 1 -
container
index 1 -
p-
index 2 -
px-
etc
variants are just a flat list e.g.
index 1 -
hover
index 2 -
focus
etc
"classname"
hover:p-4 px-2 [mask:none]
"classes"
hover:p-4
andpx-2
and[mask:none]
"lexed/tokenized classes"
hover:p-4
: varianthover
and utilityp-4
px-2
: utilitypx-2
[mask:none]
: utility[mask:none]
(arbitrary!)"class info"
hover:p-4
: variant weight 1, utility layer "utilities", utility index "1", text "hover:p-4"etc
To briefly explain this variant weight thing, each variant has a "bit" in a big bit vector, e.g. in my example sort config:
hover
:01
focus
:10
When computed the "total weight", it's just a bitwise "or" between all weights, e.g.
hover:px-2
:01
hover:focus:px-2
:11
focus:px-2
:10
px-2
:00
Which means that the order (given that the utility is the same) will be:
px-2 hover:px-2 focus:px-2 hover:focus:px-2
Why
Many people use utility classes / atomic CSS. In a nutshell, it means using classes that do one thing (e.g.
.m-4
sets amargin
). There are a few ways in which these are used:When authoring in this way, the HTML/JSX code contains elements/components with potentially many classes, e.g.:
This can get a bit messy, and that's why https://github.com/heybourn/headwind, a tool to sort them in a deterministic way, was created. Then, the Tailwind CSS folks came out with their version in the form of a Prettier plugin, which is widely used these days: https://github.com/tailwindlabs/prettier-plugin-tailwindcss
Many developers rely on it, and many have manifested that they are waiting for Biome to add similar functionality before switching from Prettier. This PR aims to do just that.
Utility classes / atomic CSS
I won't go in-depth here as I think utility classes are best explained in the Tailwind CSS docs or any article out there. In this section, I'm going to focus on everything relevant to class sorting.
There are two main concepts: utilities and variants.
.relative
forposition: relative
.hover:bg-red-500
results in a red background when the element is hovered. Variants can be stacked, e.g.md:focused:w-full
results inwidth: 100%
when the element is focused and when the screen width exceeds the value for the "medium" breakpoint.Arbitrary values (
top-[13px]
), arbitrary CSS ([mask-type:luminance]
) and arbitrary variants ([&:nth-child(3)]:underline
) are also possible.The three projects mentioned follow this structure. Nativewind works almost exactly like Tailwind CSS (in fact, it uses it under the hood). Uno CSS is more of a "make your own Tailwind" project, with a lot of presets, including a tailwind compat one.
How the Talwind sorting algorithm works
Utilities
The order is pretty straightforward and depends on a specific index per utility. For example, utility
a
might have an index of1
, and utilityb
might have an index of2
. So, something likea-24
will go beforeb-something
.Utilities can have one or both of these traits:
-suffix
. E.g.a-12
,b-something
, etc.c
.Real-world examples:
p-2
,mt-6
.block
,sticky
,underline
.grow
,grow-2
,border
,border-4
.If the utility has both traits then the "default" goes first when sorted, e.g.
a a-12
.These three pieces of information (index, has default, has values) are easy to obtain from the tailwind config file by using some tools exported by the tailwind package (already used by other packages like the prettier plugin itself). I already have a working script that does this, and turn it into a more compact format that I will discuss below.
Variants
These are a bit trickier, but after a long night, I was able to work it out and replicate it from scratch 😅
Variants are never alone, as they are "modifiers" for utilities, so they always come with a utility, e.g.
hover:bg-red-500
. Furthermore, they can be stacked, e.g.md:hover:bg-green-500
.Tailwind assigns a big number (JS bigint type) to each variant (it calls this number
variants
, I'm gonna call it "weight" because it's more descriptive for this explanation). This "weight" number is crucial for sorting.This is how the comparison function of the sorting algorithm works in respect to classes:
a | b | c
where a, b and c are the weights that correspond to each variant).For this algorithm, the necessary information is this variant "weight", which can also be obtained from the tailwind config file, in the same way as described above.
Advanced details
Note that I'm omitting some information to make things easier to understand at first glance. Some details might not be worth talking about, but here's a list with other considerations:
.container
- all other classes are utilities).screen
variants, likesm
,md
,lg
, etc). There are other details of the sorting algorithm that might or might not be worth porting over: layers, parent layers, "parallel indexes" (whatever that might be)...WIP
You've reached the end, for now. I will expand on this soon, with details about this implementation, ideas, and more.
Check again soon :)