fix(organizeImports): sort specifiers in bare exports#9353
Open
fix(organizeImports): sort specifiers in bare exports#9353
Conversation
🦋 Changeset detectedLatest commit: 7c1b786 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 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 |
github-actions
bot
added
A-Linter
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds sorting support for named specifiers in bare export statements and wires it into the organize_imports flow. Introduces public modules 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_js_analyze/src/assist/source/organize_imports.rs`:
- Around line 857-864: The Issue::UnorganizedItem constructed when an export has
no `from` clause should not set are_attributes_unsorted to true; update the
UnorganizedItem creation in the block that pushes for exports (the one using
item.syntax().index(), are_specifiers_unsorted, and NewLineIssue::None) to set
are_attributes_unsorted: false to reflect that clause.attribute() is None and no
attributes exist to be unsorted.
In
`@crates/biome_js_analyze/src/assist/source/organize_imports/specifiers_attributes.rs`:
- Around line 239-276: The function merge_export_specifiers is dead code (the
active merge path uses merge_export_from_specifiers) — either delete
merge_export_specifiers to remove unused code or keep it but document why (e.g.,
reserved for future “bare export” merging) and mark it as intentionally unused;
update the function comment to mention its relation to
merge_export_from_specifiers and the sort_export_specifiers behavior for
JsExportNamedSpecifierList so future readers understand its purpose, or remove
all references to it to eliminate the dead function entirely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 530e72ef-e2a4-4de2-b63f-c5b14637827b
⛔ Files ignored due to path filters (1)
crates/biome_js_analyze/tests/specs/source/organizeImports/unsorted-from-less-export.js.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (4)
.changeset/neat-papers-think.mdcrates/biome_js_analyze/src/assist/source/organize_imports.rscrates/biome_js_analyze/src/assist/source/organize_imports/specifiers_attributes.rscrates/biome_js_analyze/tests/specs/source/organizeImports/unsorted-from-less-export.js
crates/biome_js_analyze/src/assist/source/organize_imports/specifiers_attributes.rs
Outdated
Show resolved
Hide resolved
Conaclos
force-pushed
the
conaclos/sort-bare-exports
branch
from
168a2da to
265367e
Compare
Conaclos
force-pushed
the
conaclos/sort-bare-exports
branch
2 times, most recently
from
64f6251 to
78d6feb
Compare
Merging this PR will not alter performance
Comparing 
Footnotes
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/biome_lsp/src/server.tests.rs (1)
2019-2019: Keep this fixture unsorted to preserve LSP coverage of the new behaviour.Line 2019 is now already sorted, so this test no longer proves that
organizeImportssorts bare export specifiers end-to-end. Consider keeping it unsorted (or adding a dedicated LSP case) and asserting the extra edit, so future regressions are caught.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_lsp/src/server.tests.rs` at line 2019, The test fixture's export line "export { describe, test, z }" is now sorted which removes coverage for LSP's organizeImports behavior; change that line back to an unsorted order (e.g., "export { z, test, describe }" or similar) in the test fixture so the organizer must reorder it, and update the corresponding test assertions in the same test case (the one that triggers organizeImports) to expect the extra edit produced by the LSP (assert the additional textEdit/WorkspaceEdit that sorts the bare export specifiers). Ensure you update any helper names referenced in the test case that locate the edits so the assertion targets the correct edit produced by organizeImports.crates/biome_js_analyze/src/assist/source/organize_imports.rs (1)
1177-1181: Minor typo polish:meregd_*→merged_*.Pure readability, but worth fixing while touching this path.
Suggested diff
- if let Some(meregd_specifiers) = + if let Some(merged_specifiers) = merge_export_from_specifiers(&specifiers1, &specifiers2, sort_order) { - let meregd_clause = clause1.with_specifiers(meregd_specifiers); - let merged_item = item2.clone().with_export_clause(meregd_clause.into()); + let merged_clause = clause1.with_specifiers(merged_specifiers); + let merged_item = item2.clone().with_export_clause(merged_clause.into());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/assist/source/organize_imports.rs` around lines 1177 - 1181, There are typos in the temporary variable names: rename meregd_specifiers to merged_specifiers, meregd_clause to merged_clause, and update any usages (e.g., where meregd_clause is created from clause1 and where merged_item is created using item2.with_export_clause) so they consistently use merged_* names; keep the logic in merge_export_from_specifiers(specifiers1, specifiers2, sort_order) and the subsequent with_specifiers/with_export_clause calls unchanged, only correct the identifier spellings.crates/biome_js_analyze/src/assist/source/organize_imports/specifiers_attributes.rs (1)
20-27: Tiny naming tidy-up: fixspecifeirstypo for readability.No functional impact, but this typo appears in multiple match arms and makes scans a touch harder.
Suggested diff
- Self::JsNamedImportSpecifiers(specifeirs) => { - are_import_specifiers_sorted(specifeirs, sort_order) + Self::JsNamedImportSpecifiers(specifiers) => { + are_import_specifiers_sorted(specifiers, sort_order) } - Self::JsExportNamedFromSpecifierList(specifeirs) => { - are_export_from_specifiers_sorted(specifeirs, sort_order) + Self::JsExportNamedFromSpecifierList(specifiers) => { + are_export_from_specifiers_sorted(specifiers, sort_order) } - Self::JsExportNamedSpecifierList(specifeirs) => { - are_export_specifiers_sorted(specifeirs, sort_order) + Self::JsExportNamedSpecifierList(specifiers) => { + are_export_specifiers_sorted(specifiers, sort_order) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/assist/source/organize_imports/specifiers_attributes.rs` around lines 20 - 27, Rename the misspelled local variable `specifeirs` to `specifiers` in the match arms for Self::JsNamedImportSpecifiers, Self::JsExportNamedFromSpecifierList, and Self::JsExportNamedSpecifierList so the bindings read e.g. `Self::JsNamedImportSpecifiers(specifiers)` and update the corresponding uses in the calls to are_import_specifiers_sorted, are_export_from_specifiers_sorted, and are_export_specifiers_sorted to pass `specifiers`; this is purely a rename for readability so ensure all occurrences in that match branch are updated consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/biome_js_analyze/src/assist/source/organize_imports.rs`:
- Around line 1177-1181: There are typos in the temporary variable names: rename
meregd_specifiers to merged_specifiers, meregd_clause to merged_clause, and
update any usages (e.g., where meregd_clause is created from clause1 and where
merged_item is created using item2.with_export_clause) so they consistently use
merged_* names; keep the logic in merge_export_from_specifiers(specifiers1,
specifiers2, sort_order) and the subsequent with_specifiers/with_export_clause
calls unchanged, only correct the identifier spellings.
In
`@crates/biome_js_analyze/src/assist/source/organize_imports/specifiers_attributes.rs`:
- Around line 20-27: Rename the misspelled local variable `specifeirs` to
`specifiers` in the match arms for Self::JsNamedImportSpecifiers,
Self::JsExportNamedFromSpecifierList, and Self::JsExportNamedSpecifierList so
the bindings read e.g. `Self::JsNamedImportSpecifiers(specifiers)` and update
the corresponding uses in the calls to are_import_specifiers_sorted,
are_export_from_specifiers_sorted, and are_export_specifiers_sorted to pass
`specifiers`; this is purely a rename for readability so ensure all occurrences
in that match branch are updated consistently.
In `@crates/biome_lsp/src/server.tests.rs`:
- Line 2019: The test fixture's export line "export { describe, test, z }" is
now sorted which removes coverage for LSP's organizeImports behavior; change
that line back to an unsorted order (e.g., "export { z, test, describe }" or
similar) in the test fixture so the organizer must reorder it, and update the
corresponding test assertions in the same test case (the one that triggers
organizeImports) to expect the extra edit produced by the LSP (assert the
additional textEdit/WorkspaceEdit that sorts the bare export specifiers). Ensure
you update any helper names referenced in the test case that locate the edits so
the assertion targets the correct edit produced by organizeImports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 96fa58fd-7929-48df-8d3d-a40ed49d44f2
⛔ Files ignored due to path filters (1)
crates/biome_js_analyze/tests/specs/source/organizeImports/unsorted-from-less-export.js.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (5)
.changeset/neat-papers-think.mdcrates/biome_js_analyze/src/assist/source/organize_imports.rscrates/biome_js_analyze/src/assist/source/organize_imports/specifiers_attributes.rscrates/biome_js_analyze/tests/specs/source/organizeImports/unsorted-from-less-export.jscrates/biome_lsp/src/server.tests.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/neat-papers-think.md
- crates/biome_js_analyze/tests/specs/source/organizeImports/unsorted-from-less-export.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix partially #7583
This addresses the main issue which is sorting named specifiers of bare exports.
Here is an example of teh code action:
Test Plan
I added a test
Docs
I added a changeset.