feat(analyze/json): useSortedPackageJson#9134
Conversation
🦋 Changeset detectedLatest commit: 72784d3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Merging this PR will not alter performance
Comparing Footnotes
|
Co-authored-by: Arthur Fiorette <[email protected]>
Co-authored-by: Arthur Fiorette <[email protected]>
Co-authored-by: Arthur Fiorette <[email protected]>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new assist action Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
crates/biome_json_analyze/src/lint/nursery/use_required_scripts.rs (1)
106-115: Consider avoiding intermediateVec<String>allocation.Per repository guidance on avoiding string allocations for comparisons, you could skip collecting into
existing_scriptsand compare directly usingTokenText:♻️ Suggested refactor
- let existing_scripts: Vec<String> = scripts_object - .json_member_list() - .iter() - .flatten() - .filter_map(|member| { - let name = member.name().ok()?; - let text = name.inner_string_text()?; - Some(text.text().to_string()) - }) - .collect(); - - let missing_scripts: Vec<String> = options + let members = scripts_object.json_member_list(); + let missing_scripts: Vec<String> = options .required_scripts .iter() - .filter(|script| !existing_scripts.iter().any(|s| s == *script)) + .filter(|script| { + !members.iter().flatten().any(|member| { + member + .name() + .ok() + .and_then(|n| n.inner_string_text()) + .is_some_and(|text| text.text() == script.as_str()) + }) + }) .cloned() .collect();Based on learnings: "Avoid string allocations by using
&strorTokenTextfor comparisons instead of callingto_string()".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_json_analyze/src/lint/nursery/use_required_scripts.rs` around lines 106 - 115, The code currently builds an intermediate Vec<String> called existing_scripts by calling to_string() on each member name; instead avoid the allocation by operating on TokenText/&str directly — e.g., use scripts_object.json_member_list().iter().flatten().filter_map(|member| member.name().ok()?.inner_string_text().map(|t| t.text())).then either collect into a HashSet<&TokenText> or use the iterator with any/contains against the TokenText/&str you need to compare; update places referencing existing_scripts to use the new iterator/collection so no to_string() allocations occur (look for symbols scripts_object, json_member_list, existing_scripts, and name()/inner_string_text()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_analyze/src/rule.rs`:
- Line 460: The URL mapped for Self::SortPackageJson is wrong (it's not an npm
CLI command); update the mapping in rule.rs so SortPackageJson points to the
third‑party package page (for example the npm package URL
"https://www.npmjs.com/package/sort-package-json" or the project repo
"https://github.com/keithamus/sort-package-json") by replacing the existing
string for Self::SortPackageJson with the correct package URL.
In `@crates/biome_json_analyze/src/assist/source/use_sorted_package_json.rs`:
- Around line 370-405: The current organize_members function uses a
HashMap<TokenText, JsonMember> (member_map) which collapses duplicate keys and
loses members; instead collect a Vec of (TokenText, JsonMember) from the
incoming JsonMemberList (preserving duplicates and original occurrence order),
then build a grouping (e.g., HashMap<TokenText, Vec<JsonMember>> or better an
ordered map like Vec<(TokenText, Vec<JsonMember>)>) so each key maps to a queue
of all members; iterate sorted_names from get_sorted_field_order and for each
name drain/iterate its Vec of JsonMember values, applying
apply_field_transformer to each member and pushing them onto elements (adding
separators between entries), and return make::json_member_list(elements,
separators); do not replace member_map with a single-value map that drops
duplicates and ensure JsonMemberList ordering for duplicate keys is preserved.
In
`@crates/biome_json_analyze/src/assist/source/use_sorted_package_json/sorters/dependencies_meta.rs`:
- Around line 37-81: The transform function currently bails out when
sort_object_by_comparator returns None so nested objects never get deep‑sorted;
update transform to always call deep_sort_nested_objects on the object (use the
sorted object when sort_object_by_comparator returns Some or the original object
when it returns None) and only return Some(AnyJsonValue::from(...)) if either
the top-level sort or the deep sort produced changes. Also change
deep_sort_nested_objects to return None when it detects no changes (instead of
Some(object.clone())), using super::helpers::sort_alphabetically_deep to detect
nested changes; reference functions: transform, sort_object_by_comparator,
deep_sort_nested_objects, get_package_name, and
super::helpers::sort_alphabetically_deep.
In
`@crates/biome_json_analyze/src/assist/source/use_sorted_package_json/sorters/prettier_config.rs`:
- Around line 5-68: Summary: transform currently sorts top-level keys but leaves
the "overrides" array and each override's "options" object unsorted; update it
to alphabetise both. Fix: in transform (function transform, variables
overrides_member, keys_with_members, make::json_member_list) when
overrides_member is Some, parse its JsonMember value as a JSON array, iterate
each array element that is an object and rebuild each override object by sorting
its members alphabetically and, if it has an "options" member, parse that
options value as an object and sort its members alphabetically too; then
reassemble a new overrides JsonMember with a json array of sorted override
objects and use that sorted overrides member when building new_members so
overrides remains last; add a unit test that supplies a prettier config with
unsorted overrides and options and asserts alphabetical ordering.
In `@crates/biome_json_analyze/tests/compat_sortpkg_tests.rs`:
- Around line 210-277: The test function test_compat_with_sortpkg currently
prints a success message with eprintln!("All {total} sort-package-json
compatibility tests passed"); replace that debug output by using dbg!(...) or
remove it per guideline: change the eprintln! call to a dbg!(format!("All {}
sort-package-json compatibility tests passed", total)) (or simply drop the
success print) so the test uses the dbg! macro for debugging output instead of
eprintln!.
---
Nitpick comments:
In `@crates/biome_json_analyze/src/lint/nursery/use_required_scripts.rs`:
- Around line 106-115: The code currently builds an intermediate Vec<String>
called existing_scripts by calling to_string() on each member name; instead
avoid the allocation by operating on TokenText/&str directly — e.g., use
scripts_object.json_member_list().iter().flatten().filter_map(|member|
member.name().ok()?.inner_string_text().map(|t| t.text())).then either collect
into a HashSet<&TokenText> or use the iterator with any/contains against the
TokenText/&str you need to compare; update places referencing existing_scripts
to use the new iterator/collection so no to_string() allocations occur (look for
symbols scripts_object, json_member_list, existing_scripts, and
name()/inner_string_text()).
a507e67 to
ad46c7b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/biome_json_analyze/src/assist/source/use_sorted_package_json/sorters/helpers.rs (1)
237-264: Consider usingrustc_hash::FxHashSetfor consistency.The crate already depends on
rustc-hashper the library context. UsingFxHashSetinstead ofstd::collections::HashSetwould be more consistent and potentially faster for string keys.Suggested change
+use rustc_hash::FxHashSet; + /// Remove duplicate string values from an array pub fn uniq_array(array: &AnyJsonValue) -> Option<AnyJsonValue> { let array_value = array.as_json_array_value()?; let elements = array_value.elements(); - let mut seen: std::collections::HashSet<TokenText> = std::collections::HashSet::new(); + let mut seen: FxHashSet<TokenText> = FxHashSet::default();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_json_analyze/src/assist/source/use_sorted_package_json/sorters/helpers.rs` around lines 237 - 264, The function uniq_array uses std::collections::HashSet for seen; replace it with rustc_hash::FxHashSet to match the crate's dependency and improve performance: import rustc_hash::FxHashSet (or prefix it as rustc_hash::FxHashSet) and change the variable declaration from std::collections::HashSet<TokenText> to FxHashSet<TokenText>, and construct it with FxHashSet::default() or FxHashSet::with_capacity(...) instead of HashSet::new(); keep all other logic in uniq_array (and references to TokenText) unchanged.crates/biome_json_analyze/tests/compat_sortpkg/data.json (2)
40-42: Duplicate test names may cause confusion.Lines 22 and 41 both use
"Should sort \dependenciesMeta` as object."Similarly, multipleexports` tests share identical names (lines 227, 246, 265, etc.). Consider adding distinguishing suffixes (e.g., "with scoped packages", "with versioned keys") to aid debugging when tests fail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_json_analyze/tests/compat_sortpkg/data.json` around lines 40 - 42, The JSON test entries in data.json have duplicate "testName" values (e.g., multiple "Should sort `dependenciesMeta` as object." and repeated "exports" test names), which makes failing tests ambiguous; update each offending test object by making its "testName" unique—append a short distinguishing suffix such as "—with scoped packages", "—with versioned keys", or "—case X" to the "testName" field for the entries that test different scenarios so each test can be identified unambiguously (locate and edit the "testName" properties in the JSON objects that start with "Should sort `dependenciesMeta` as object." and the repeated "exports" tests).
183-184: Clarify the"input": nullpattern.Several test cases use
"input": null(lines 184, 391, 415, 687, 711). If this means "output serves as both input and expected result", consider adding a comment at the top of the file or in a README explaining this convention for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_json_analyze/tests/compat_sortpkg/data.json` around lines 183 - 184, Several test entries in data.json use the pattern "input": null to indicate that the test's output value should be used as the input (i.e., expected result doubles as input); add a short clarifying note at the top of this JSON test data file (or in an adjacent README) that documents this convention and lists the meaning of "input": null, referencing the "testName" fields (for example entries like "Should sort `eslintConfig.override[]` same as `eslintConfig`") and the "input"/"output" keys so future maintainers understand that null input means "use output as input."
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/biome_analyze/src/rule.rs`:
- Line 460: The URL for Self::SortPackageJson is incorrect (points to a
non-existent npm CLI command); update the string returned for the
SortPackageJson variant to the official package page (for example
"https://www.npmjs.com/package/sort-package-json" or the GitHub repo
"https://github.com/keithamus/sort-package-json") in the match/impl handling the
rule URLs so the variant Self::SortPackageJson returns the correct third-party
package URL.
In `@crates/biome_json_analyze/src/assist/source/use_sorted_package_json.rs`:
- Around line 370-405: The current organize_members collects members into a
HashMap<TokenText, JsonMember> which drops duplicate keys; change this to
collect into a HashMap<TokenText, Vec<JsonMember>> (or similar per-key queue) so
all JsonMember occurrences are preserved in insertion order, then when iterating
sorted_names fetch the Vec for that key and push each member (applying
apply_field_transformer to each) into elements in the original occurrence order,
adding separators between members as before and finally build the JsonMemberList
via make::json_member_list; keep using extract_field_names and
get_sorted_field_order for the ordering but replace the single-value member_map
with a per-key list to avoid collapsing duplicates.
In
`@crates/biome_json_analyze/src/assist/source/use_sorted_package_json/sorters/dependencies_meta.rs`:
- Around line 37-81: transform currently short‑circuits when
sort_object_by_comparator returns None and deep_sort_nested_objects always
returns Some even if unchanged; change transform to always obtain a
JsonObjectValue to pass into deep_sort_nested_objects (use the sorted_top_level
if Some or the original object.clone() if None) so nested objects are inspected
even when top-level ordering didn’t change, and modify deep_sort_nested_objects
to return None when no nested changes were made (only return
Some(updated_object) when any member was replaced with a deep-sorted value);
reference functions: transform, sort_object_by_comparator,
deep_sort_nested_objects, and super::helpers::sort_alphabetically_deep.
In `@crates/biome_json_analyze/tests/compat_sortpkg_tests.rs`:
- Around line 210-277: In test_compat_with_sortpkg replace the debug eprintln!
call that prints the success message with either a dbg! invocation or remove it:
locate the eprintln!("All {total} sort-package-json compatibility tests passed")
in the test_compat_with_sortpkg function and change it to dbg!(format!("All {}
sort-package-json compatibility tests passed", total)) (or simply delete the
line) so debug output uses dbg! or is dropped per guidelines.
---
Nitpick comments:
In
`@crates/biome_json_analyze/src/assist/source/use_sorted_package_json/sorters/helpers.rs`:
- Around line 237-264: The function uniq_array uses std::collections::HashSet
for seen; replace it with rustc_hash::FxHashSet to match the crate's dependency
and improve performance: import rustc_hash::FxHashSet (or prefix it as
rustc_hash::FxHashSet) and change the variable declaration from
std::collections::HashSet<TokenText> to FxHashSet<TokenText>, and construct it
with FxHashSet::default() or FxHashSet::with_capacity(...) instead of
HashSet::new(); keep all other logic in uniq_array (and references to TokenText)
unchanged.
In `@crates/biome_json_analyze/tests/compat_sortpkg/data.json`:
- Around line 40-42: The JSON test entries in data.json have duplicate
"testName" values (e.g., multiple "Should sort `dependenciesMeta` as object."
and repeated "exports" test names), which makes failing tests ambiguous; update
each offending test object by making its "testName" unique—append a short
distinguishing suffix such as "—with scoped packages", "—with versioned keys",
or "—case X" to the "testName" field for the entries that test different
scenarios so each test can be identified unambiguously (locate and edit the
"testName" properties in the JSON objects that start with "Should sort
`dependenciesMeta` as object." and the repeated "exports" tests).
- Around line 183-184: Several test entries in data.json use the pattern
"input": null to indicate that the test's output value should be used as the
input (i.e., expected result doubles as input); add a short clarifying note at
the top of this JSON test data file (or in an adjacent README) that documents
this convention and lists the meaning of "input": null, referencing the
"testName" fields (for example entries like "Should sort
`eslintConfig.override[]` same as `eslintConfig`") and the "input"/"output" keys
so future maintainers understand that null input means "use output as input."
|
Sorry for not having time to fix all the past reviews :/ Work is demanding a lot for the last weeks... Thanks for picking this up, great work!! |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_analyze/src/rule.rs`:
- Around line 294-297: The variant_index mapping currently assigns the same
index 38 to Self::HtmlEslint and Self::EslintE18e, causing duplicate indices;
update the mapping so each enum variant has a unique sequential index by
changing Self::EslintE18e to return 39 (and increment subsequent indices
accordingly—e.g., change Self::EslintBetterTailwindcss to 40 and
Self::SortPackageJson to 41) in the function/match that implements variant_index
to restore deterministic ordering.
In `@crates/biome_json_analyze/src/assist/source/use_sorted_keys.rs`:
- Around line 214-216: The comment incorrectly states we "Skip package.json
files - they have specialized sorting via organizePackageJson" while the code
only retrieves the path (ctx.file_path()) and proceeds with the
useSortedPackageJson action; either implement an actual skip or update the
comment to match behavior — I recommend updating the comment to something like
"Handle package.json with useSortedPackageJson note" or, if you want to skip,
add a guard that checks the filename (e.g., if ctx.file_path().file_name() ==
Some("package.json")) and branch to organizePackageJson or return early;
reference use_sorted_keys.rs, ctx.file_path(), organizePackageJson and
useSortedPackageJson when making the change.
In
`@crates/biome_json_analyze/src/assist/source/use_sorted_package_json/sorters/helpers.rs`:
- Around line 274-320: The function uniq_and_sort_array currently moves all
non-string elements to the end (non_string_elements) which changes their
relative positions in mixed-type arrays; instead detect mixed arrays and bail
out to avoid reordering: in uniq_and_sort_array, after collecting string_values
and non_string_elements, if both collections are non-empty (i.e., the array
contains at least one string and at least one non-string), return None (do not
modify) so positions are preserved; keep this check before deduping/sorting and
before calling rebuild_array.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
crates/biome_analyze/src/rule.rs (1)
183-184: Minor doc-comment style inconsistency.All other variants follow
/// Rules from [Name](url)with a markdown hyperlink. This one uses a raw URL and the atypical phrasing "Action for", which will look odd on the generated docs page.✏️ Suggested tweak
- /// Action for https://github.com/keithamus/sort-package-json + /// Assist action from [sort-package-json](https://github.com/keithamus/sort-package-json) SortPackageJson,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_analyze/src/rule.rs` around lines 183 - 184, The doc-comment for the enum variant SortPackageJson is inconsistent with others; update its comment to match the pattern used elsewhere (e.g., "/// Rules from [sort-package-json](https://github.com/keithamus/sort-package-json)") so it uses a markdown hyperlink and the same "Rules from [Name](url)" phrasing as the other variants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@crates/biome_json_analyze/src/assist/source/use_sorted_package_json/sorters/helpers.rs`:
- Around line 58-92: Replace the plain comment above sort_object_by_comparator
with a rustdoc doctest: add triple-slash /// and a ```rust code block showing a
minimal, self-contained example that constructs a small JsonObjectValue (or uses
a helper test fixture available in the crate), calls sort_object_by_comparator
with a simple comparator (e.g., |a,b| a.cmp(b)), and asserts the returned Option
is Some(...) when keys reorder (or None when already sorted), so the example
compiles and the assertions pass; keep the doc example minimal and reference
sort_object_by_comparator by name in the doc comment.
- Around line 6-12: Replace the prose doc comment for extract_member_names with
a rustdoc doctest: wrap an example in a /// ```rust ... /// ``` fenced code
block that constructs a small slice of JsonMember values, calls
extract_member_names(&members), and asserts the returned Vec<TokenText> equals
the expected tokens (use assert_eq!). Ensure the doctest includes any necessary
use/imports or fully-qualified type names (JsonMember, TokenText) so it compiles
in tests and demonstrates expected behavior.
- Around line 274-327: The rustdoc above uniq_and_sort_array should be converted
into a doctest example: replace the existing plain comment with a /// code block
using ```rust that demonstrates calling uniq_and_sort_array with a
representative AnyJsonValue (including mixed-type check) and an assert! (or
assert_eq!) that verifies the returned Option/AnyJsonValue matches the expected
sorted/unique array; ensure the example builds by importing or constructing the
necessary AnyJsonValue/TokenText helpers in the doctest and that it exercises
both the string-only path and the mixed-type bail-out path so the assertions
pass when running cargo test.
- Around line 244-272: The top-line rustdoc for the function uniq_array is plain
text; convert it into a doctest by replacing the comment with a rustdoc code
block demonstrating expected behavior: add an example under the /// comment
using /// ```rust ... /// ``` that constructs an AnyJsonValue array (use
AnyJsonValue helpers or a minimal example), calls uniq_array(&array) and asserts
the returned Option contains the rebuilt array via rebuild_array or expected
JSON (use assert!(uniq_array(...).is_some()) and assert_eq!(...)). Ensure the
doctest imports or references types/functions used (AnyJsonValue, uniq_array,
rebuild_array) so the example compiles and assertions validate the deduplication
behavior.
In
`@crates/biome_json_analyze/src/assist/source/use_sorted_package_json/sorters/prettier_config.rs`:
- Around line 120-164: Replace the existing plain comment above
sort_override_entry with a rustdoc doctest: add a /// doc comment block directly
above fn sort_override_entry showing a minimal code example that constructs a
sample JsonObjectValue override (including an "options" sub-object), calls
sort_override_entry, and asserts the returned tuple equals the expected sorted
JsonObjectValue and changed boolean (use assert_eq! to validate both result and
changed); ensure the code block is fenced as ```rust and compiles in doctest by
importing any needed types or using helper constructors used elsewhere in this
module so the assertions pass.
---
Nitpick comments:
In `@crates/biome_analyze/src/rule.rs`:
- Around line 183-184: The doc-comment for the enum variant SortPackageJson is
inconsistent with others; update its comment to match the pattern used elsewhere
(e.g., "/// Rules from
[sort-package-json](https://github.com/keithamus/sort-package-json)") so it uses
a markdown hyperlink and the same "Rules from [Name](url)" phrasing as the other
variants.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/biome_analyze/src/rule.rscrates/biome_json_analyze/src/assist/source/use_sorted_keys.rscrates/biome_json_analyze/src/assist/source/use_sorted_package_json/sorters/helpers.rscrates/biome_json_analyze/src/assist/source/use_sorted_package_json/sorters/prettier_config.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_json_analyze/src/assist/source/use_sorted_keys.rs
Summary
Superseeds #8659 with all the applied feedback from us.
I changed the API signature of some functions. Some function were returning
Option<Result<>>, which is an awkward, and useless API. Now it resultsOption.With the help of a coding agent:
Test Plan
All tests should pass
Docs