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

feat(biome_css_analyze): add rule useSortedProperties #4063

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

simon-paris
Copy link
Contributor

@simon-paris simon-paris commented Sep 24, 2024

Summary

Related: #3 (comment), #3167

This implements CSS declaration and rule sorting from the stylelint-order plugin. It sorts properties (and nested rules) in a consistent order, e.g:

  ! Properties should be sorted: display should be before color.
  
  > 1 │ .class { color: red; display: block; }
       │       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  
  i Safe fix: Sort these properties
  
    1    │ - .class·{·color:·red;·display:·block;·}
       1 │ + .class·{·display:·block;·color:·red;·}

The property order comes from the most popular preset config: stylelint-config-recess-order.

This is the equivalent stylelint config:

{
	extends: ['stylelint-config-recess-order'],
	rules: {
		order/order: [
		        "custom-properties",
		        "declarations",
		        "rules",
		        "at-rules"
	        ]
	},
}

The sort order is this: (ties stay in the same relative order)

  1. Node kind:
    a. Custom properties
    b. The "composes" property
    c. Properties
    e. Nested rules and at-rules
    f. Other
  2. Property name:
    a. By the order defined in PROPERTY_ORDER
  3. Vendor prefix:
    a. By the order defined in VENDOR_PREFIXES
    b. Properties with no vendor prefix

Some possible issues:

  • This is the only CSS rule that has a quickfix. I'm not sure if quickfixing CSS is stable. Seems to work though.
  • stylelint-config-recess-order has an open PR that changes the order a little bit. It'd be best to wait for that to merge and sync the ordering before promoting this rule.
  • stylelint-config-recess-order doesn't list every property. There are 230 (rarely used) properties that have no defined order. I'd rather not unilaterally decide where they go so I've left them out for now. This rule silences itself if any property exists that isn't in the list. The stylelint version doesn't raise errors for unknown properties but applying any autofix will move them to the end.
  • I've had to duplicate some logic from the noShorthandPropertyOverrides rule because this rule would apply incorrect quickfixes in that case. I've made this rule silence itself if that rule would have fired. Not sure if that's the right approach. The stylelint plugin doesn't do this and applies the fix even when its unsafe.
  • I've used LazyLock<HashMap> to implement some lookup tables for this rule. Not sure if that's a good method, I'm new to rust.

Test Plan

Unit tests are included.

@github-actions github-actions bot added A-CLI Area: CLI A-Project Area: project A-Linter Area: linter L-JavaScript Language: JavaScript and super languages L-CSS Language: CSS A-Diagnostic Area: diagnostocis labels Sep 24, 2024
@simon-paris simon-paris changed the title Feat/use sorted properties feat(css_analyze): Add useSortedProperties rule Sep 24, 2024
@simon-paris simon-paris changed the title feat(css_analyze): Add useSortedProperties rule feat(biome_css_analyze): Add useSortedProperties rule Sep 24, 2024
@simon-paris simon-paris changed the title feat(biome_css_analyze): Add useSortedProperties rule feat(biome_css_analyze): add rule useSortedProperties Sep 24, 2024
@simon-paris simon-paris changed the title feat(biome_css_analyze): add rule useSortedProperties feat(biome_css_analyze): add rule useSortedProperties Sep 24, 2024
Copy link

codspeed-hq bot commented Oct 14, 2024

CodSpeed Performance Report

Merging #4063 will degrade performances by 19.89%

Comparing simon-paris:feat/use-sorted-properties (8bbed82) with main (e3e37ab)

Summary

❌ 3 regressions
✅ 94 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main simon-paris:feat/use-sorted-properties Change
css_analyzer[bulma_5641719244145477318.css] 621.4 ms 718 ms -13.45%
css_analyzer[foundation_11602414662825430680.css] 149.6 ms 186.7 ms -19.89%
css_analyzer[tachyons_11778168428173736564.css] 122 ms 142.6 ms -14.42%

@ematipico
Copy link
Member

Thank you @simon-paris for the contribution. I will look into this rule this week. As a very early feedback, this rule should be made an assist (apologies for the lack of documentation, I will try to fix that ASAP).

Here's an example of assist: https://github.com/biomejs/biome/blob/main/crates/biome_json_analyze/src/assists/source/use_sorted_keys.rs

@@ -3436,6 +3436,10 @@ pub struct Nursery {
#[serde(skip_serializing_if = "Option::is_none")]
pub use_sorted_classes:
Option<RuleFixConfiguration<biome_js_analyze::options::UseSortedClasses>>,
#[doc = "Enforce ordering of CSS properties."]
#[serde(skip_serializing_if = "Option::is_none")]
Copy link

Choose a reason for hiding this comment

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

I know sorting alphabetically may have issues (example) but I'd really appreciate having an option for it. For me it's much more useful than the "recess-order" configuration. Would it be difficult to add support for it?

Copy link
Member

Choose a reason for hiding this comment

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

What's a "recess-order"?

Copy link

Choose a reason for hiding this comment

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

The grouped order that stylelint-config-recess-order uses by default, and the one this PR was based on.

Copy link

@jeddy3 jeddy3 Nov 28, 2024

Choose a reason for hiding this comment

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

I'd appreciate having an option, too.

It looks like stylelint-config-recess-order was chosen because it's the most popular shared config. However, the Stylelint order plugin has a dedicated rule for alphabetically sorting, and that may not have been taken into account. It's difficult to compare its popularity with those of the shared configs, as they have their own npm download stats.

The dedicated rule is used in 2.1k public configs, so a lot of people prefer to sort alphabetically. Probably because there are 600+ properties, with more added all the time, so it's hard to maintain meaningful categories for all these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI A-Diagnostic Area: diagnostocis A-Linter Area: linter A-Project Area: project L-CSS Language: CSS L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants