feat(biome_js_analyzer): useJsxSortProps #2652

vohoanglong0107 · 2024-04-30T01:24:40Z

Summary

Implement Eslint's lint rule [jsx-sort-props](https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/jsx-sort-props.md] as an assist

Test Plan

All tests should pass

codspeed-hq · 2024-04-30T02:30:56Z

CodSpeed Performance Report

Merging #2652 will not alter performance

_{Comparing vohoanglong0107:feat-use-jsx-sort-props (49ad1ed) with main (88f1d5e)}

Summary

✅ 101 untouched benchmarks

ematipico · 2024-05-01T07:33:19Z

I'm not very fond of having stylistic rules inside our linter. I think this rule should be an assist.

I was working on a similar rule for sorting JSON keys, and setting up the foundations for assists inside the CLI and LSP.

vohoanglong0107 · 2024-05-01T08:10:44Z

Oh ok, then I will set this PR to draft for the time being

ematipico · 2024-08-05T14:51:58Z

Hey @vohoanglong0107, the infrastructure is ready. You should move the new rule inside src/assists/source, and use the macro declare_source_rule

PrinOrange · 2024-08-14T03:31:01Z

This rule will be helpful if it will be passed.

vohoanglong0107 · 2024-08-15T13:19:56Z

@ematipico How may I generate the configuration and the schema for an assist rule? just new-js-assistrule doesn't seem to work, the same goes for just gen-all 😥

ematipico · 2024-08-21T08:41:50Z

crates/biome_js_analyze/src/assists/source/sort_jsx_props.rs

+    /// <Hello tel={5555555} {...this.props} firstName="John" lastName="Smith" />;
+    /// ```
+    ///
+    /// ## Options


We don't have options anymore, so we should remove the docs too

ematipico · 2024-08-21T08:43:53Z

crates/biome_js_analyze/src/assists/source/sort_jsx_props.rs

+
+    fn action(ctx: &RuleContext<Self>, _state: &Self::State) -> Option<JsRuleAction> {
+        let props = ctx.query().clone();
+        let mut non_spread_props: Option<Vec<_>> = None;


Option<Vec<>> is not very easy to use. You can just use Vec<>, and if the vector is empty, it means it's like None

ematipico · 2024-08-21T08:44:41Z

crates/biome_js_analyze/src/assists/source/sort_jsx_props.rs

+    /// Enforce props sorting in JSX elements.
+    ///
+    /// This rule checks if the JSX props are sorted in a consistent way.
+    /// A spread prop resets the sorting order.
+    ///
+    /// The rule can be configured to sort props alphabetically, ignore case, and more.


There's no more configuration, however we should document very well what's the order, meaning which kinds props come first and which ones come later.

ematipico · 2024-08-21T08:47:12Z

crates/biome_js_analyze/src/assists/source/sort_jsx_props.rs

+                AnyJsxAttribute::JsxSpreadAttribute(_) => {
+                    if let Some(mut non_spread_props) = non_spread_props.take() {
+                        non_spread_props.sort_by(compare_props());
+                        new_props.extend(
+                            non_spread_props
+                                .into_iter()
+                                .map(AnyJsxAttribute::JsxAttribute),
+                        );
+                    }
+                    non_spread_props = None;
+                    new_props.push(prop);
+                }


I am not really sure I understand this part. Why do we run a sorting every time we encounter a spread prop?

The idea is that, given that a spread props resets the sorting order, every time we encounter a spread prop, we sort the previous non-spread props element.

Then, I believe the same approach we use in the organise imports would work. It makes things easier, I think.

Essentially, you create groups. Each group is delimited by a spread prop, where the spread prop is always the last one of the group.

Each group is sorted in alphabetical order (I think natural order is better), and groups aren't sorted, they are placed as they were found.

ematipico · 2024-08-21T08:48:37Z

crates/biome_js_analyze/src/assists/source/sort_jsx_props.rs

+            return None;
+        }
+        let mut mutation = ctx.root().begin();
+        mutation.replace_node_discard_trivia(props, jsx_attribute_list(new_props));


Why do we discard the trivia?

Yeah we shouldn't. My bad

ematipico · 2024-08-21T08:58:31Z

crates/biome_js_analyze/src/assists/source/sort_jsx_props.rs

+fn compare_props() -> impl FnMut(&JsxAttribute, &JsxAttribute) -> Ordering {
+    |a: &JsxAttribute, b: &JsxAttribute| -> Ordering {
+        let (Ok(a_name), Ok(b_name)) = (a.name(), b.name()) else {
+            return Ordering::Equal;
+        };
+        let (a_name, b_name) = (a_name.text(), b_name.text());
+
+        a_name.cmp(&b_name)
+    }
+}


You should take advantage of the Rust system. Rust has Ord and PartialOrd directives. Which means that the sorting will come for free once you implement the function.

Check how we do the sorting in this rule: https://github.com/biomejs/biome/blob/main/crates/biome_json_analyze/src/assists/source/use_sorted_keys.rs

We implement the ordering here:

biome/crates/biome_json_analyze/src/assists/source/use_sorted_keys.rs

Lines 29 to 37 in d079e53

impl Ord for MemberKey {

fn cmp(&self, other: &Self) -> Ordering {

// Sort keys using natural ordering

natord::compare(

&self.node.name().unwrap().text(),

&other.node.name().unwrap().text(),

)

}

}

Then, we use a is_sorted function to check if we need to run to emit an action or not:

biome/crates/biome_json_analyze/src/assists/source/use_sorted_keys.rs

Lines 105 to 109 in d079e53

if !state.is_sorted() {

Some(state)

} else {

None

}

Generally, this approach is better than the one you implemented because in this case, you always run the action, while in this case, we always run the action.

The reason I used a separate function for comparison was that, originally, I wanted to customize it with user options, and Ord doesn't support that (?). Even though this PR implements a trimmed down version without the options, I think it still makes sense to keep this in case we want to reintroduce the option later.

However, if we aren't going to support options for assist rules anytime soon, I would follow your guidance and change it to use Ord

I don't see why this wouldn't work with options. You just create a new type that holds a reference to the options other than the nodes to sort.

When implementing Ord and PartialOrd, you will read the options and change the logic accordingly

vohoanglong0107 · 2024-08-25T02:06:08Z

crates/biome_js_analyze/src/assists/source/sort_jsx_props.rs

+        let props = ctx.query().clone();
+        let mut current_prop_group = PropGroup::default();
+        let mut prop_groups = Vec::new();
+        for prop in props.clone() {
+            match prop {
+                AnyJsxAttribute::JsxAttribute(attr) => {
+                    current_prop_group.props.push(PropElement { prop: attr });
+                }
+                // spread prop reset sort order
+                AnyJsxAttribute::JsxSpreadAttribute(_) => {
+                    prop_groups.push(current_prop_group);
+                    current_prop_group = PropGroup::default();
+                }
+            }
+        }
+        prop_groups.push(current_prop_group);
+        prop_groups


This really looks cleaner after being split to groups

Vec::is_empty() is equivalent to None

Separated by spread props

ematipico

Thank you! We're almost there, we just need some final touch for the docs, then we can merge it

ematipico · 2024-08-26T13:46:14Z

crates/biome_js_analyze/src/assists/source/sort_jsx_props.rs

+    ///
+    /// This rule checks if the JSX props are sorted in a consistent way.
+    /// Props are sorted alphabetically.
+    /// A spread prop resets the sorting order.


This phrase is not very beginner-friendly. What does "resetting the sorting order" mean? I would suggest something more extensive:

Suggested change

/// A spread prop resets the sorting order.

/// When the rule encounters a spread prop, the algorithm stops ordering all the previous props. The prevents from breaking the override of certain props that rely on the order.

Please review the suggestion, and make sure that I didn't suggest anything wrong.

ematipico · 2024-08-26T13:47:07Z

crates/biome_js_analyze/src/assists/source/sort_jsx_props.rs

+    /// ```js,expect_diagnostic
+    /// <Hello lastName="Smith" firstName="John" />;
+    /// ```


It definitely makes sense to add an example with spread props, so users can understand how it works. Maybe two spread props should be enough

vohoanglong0107 · 2024-08-28T00:52:12Z

@ematipico How do you feel about the new docs?

ematipico · 2024-08-28T07:14:33Z

They're good, thank you!

github-actions bot added A-Project Area: project A-Linter Area: linter L-JavaScript Language: JavaScript and super languages A-Diagnostic Area: diagnostocis labels Apr 30, 2024

vohoanglong0107 force-pushed the feat-use-jsx-sort-props branch from c2cf7a7 to 886494a Compare April 30, 2024 01:57

vohoanglong0107 force-pushed the feat-use-jsx-sort-props branch from 886494a to 0d48b11 Compare April 30, 2024 10:19

github-actions bot added the A-CLI Area: CLI label Apr 30, 2024

vohoanglong0107 force-pushed the feat-use-jsx-sort-props branch from 870e6bf to d90a4b6 Compare April 30, 2024 13:41

vohoanglong0107 marked this pull request as ready for review April 30, 2024 13:41

vohoanglong0107 marked this pull request as draft May 1, 2024 08:10

ematipico mentioned this pull request Jun 11, 2024

☂️ Analyzer assists #3178

Open

vohoanglong0107 force-pushed the feat-use-jsx-sort-props branch from d90a4b6 to dececc2 Compare August 15, 2024 13:10

github-actions bot removed A-Project Area: project A-Diagnostic Area: diagnostocis labels Aug 15, 2024

vohoanglong0107 force-pushed the feat-use-jsx-sort-props branch from bb08a91 to 8d7f015 Compare August 21, 2024 07:44

github-actions bot added the A-Project Area: project label Aug 21, 2024

vohoanglong0107 marked this pull request as ready for review August 21, 2024 08:06

ematipico reviewed Aug 21, 2024

View reviewed changes

vohoanglong0107 force-pushed the feat-use-jsx-sort-props branch from 9671138 to 666b19e Compare August 23, 2024 08:13

vohoanglong0107 requested a review from ematipico August 24, 2024 02:33

vohoanglong0107 commented Aug 25, 2024

View reviewed changes

Vo Hoang Long added 3 commits August 25, 2024 11:57

feat(js_analyzer): sort jsx props

75ac574

docs(js_analyze): remove configuration section

7a4d280

refactor(js_analyze): use Vec<_> instead of Option<Vec<_>>

37f67c8

Vec::is_empty() is equivalent to None

Vo Hoang Long added 2 commits August 25, 2024 11:57

fix(js_analyzer): should not discard trivia

6d842eb

refactor(js_analyze): split props to group

53af54c

Separated by spread props

vohoanglong0107 force-pushed the feat-use-jsx-sort-props branch from 5e97240 to 53af54c Compare August 25, 2024 02:57

ematipico approved these changes Aug 26, 2024

View reviewed changes

docs(js_analyze): clearly explain spread prop behavior

49ad1ed

ematipico merged commit 3542e9a into biomejs:main Aug 28, 2024
12 checks passed

vohoanglong0107 deleted the feat-use-jsx-sort-props branch August 31, 2024 09:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(biome_js_analyzer): useJsxSortProps #2652

feat(biome_js_analyzer): useJsxSortProps #2652

vohoanglong0107 commented Apr 30, 2024 •

edited

Loading

codspeed-hq bot commented Apr 30, 2024 •

edited

Loading

ematipico commented May 1, 2024

vohoanglong0107 commented May 1, 2024

ematipico commented Aug 5, 2024

PrinOrange commented Aug 14, 2024

vohoanglong0107 commented Aug 15, 2024

ematipico Aug 21, 2024

ematipico Aug 21, 2024

ematipico Aug 21, 2024

ematipico Aug 21, 2024

vohoanglong0107 Aug 23, 2024

ematipico Aug 24, 2024 •

edited

Loading

ematipico Aug 21, 2024

vohoanglong0107 Aug 23, 2024

ematipico Aug 21, 2024

vohoanglong0107 Aug 23, 2024

ematipico Aug 24, 2024

vohoanglong0107 Aug 25, 2024

ematipico left a comment

ematipico Aug 26, 2024

ematipico Aug 26, 2024

vohoanglong0107 commented Aug 28, 2024

ematipico commented Aug 28, 2024

	impl Ord for MemberKey {
	fn cmp(&self, other: &Self) -> Ordering {
	// Sort keys using natural ordering
	natord::compare(
	&self.node.name().unwrap().text(),
	&other.node.name().unwrap().text(),
	)
	}
	}

	/// A spread prop resets the sorting order.
	/// When the rule encounters a spread prop, the algorithm stops ordering all the previous props. The prevents from breaking the override of certain props that rely on the order.

feat(biome_js_analyzer): useJsxSortProps #2652

feat(biome_js_analyzer): useJsxSortProps #2652

Conversation

vohoanglong0107 commented Apr 30, 2024 • edited Loading

Summary

Test Plan

codspeed-hq bot commented Apr 30, 2024 • edited Loading

Merging #2652 will not alter performance

Summary

ematipico commented May 1, 2024

vohoanglong0107 commented May 1, 2024

ematipico commented Aug 5, 2024

PrinOrange commented Aug 14, 2024

vohoanglong0107 commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico Aug 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vohoanglong0107 commented Aug 28, 2024

ematipico commented Aug 28, 2024

vohoanglong0107 commented Apr 30, 2024 •

edited

Loading

codspeed-hq bot commented Apr 30, 2024 •

edited

Loading

ematipico Aug 24, 2024 •

edited

Loading