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(organize_import): utilities for ordering import sources #4313

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

Conaclos
Copy link
Member

@Conaclos Conaclos commented Oct 16, 2024

Summary

Implements #4239.

In the RFC (#3015), I proposed to parse import sources in order to compare them in a structural way.
At the time of writing the RFC it took a lot of time trying to decide which structure to parse.
I left the RFC a bit loose because I couldn't find a satisfactory structure.

I fell into the same problem again. I am not 100% happy with the result and I am open to suggestions.
However, the current implementation should be good enough, and we can improve it later.

The implementation is in biome_js_analyze/src/assists/source/organize_imports/util.rs and biome_string_case/src/lib.rs.

In contrast to the spec, I decided to avoid a parsing phase (except for paths).
Instead, we just look at the first bytes of an import source to determine its kind.
An import source can be one of the following kind:

  • Unknown: all import paths without a recognizable kind
  • Uri, e.g. https://example.org
  • ProtocolPackage, e.g. node:test, npm:@scope/package
  • Package, e.g. package
  • Alias: Node.js subpath imports and TypeScript aliases.
    This corresponds to import sources that start with @/, #, ~, or %
    /// Import sources that start with /, ./, or ../
  • Path: import sources that start with /, ./, or ../

Import paths are first ordered according to their kind (order of kind is reflected by their position in the previous list).
Note that unrecognized import source kinds are placed first.
I am unsure if we should keep or remove Unknown and choose a kind instead (probably Alias).

Two import sources with identical kind are then ordered by a custom order:

  • Order between paths follows the proximity order (../../ < ../ < ./)
  • Other kinds use a string collation (ImportSourceAsciiCollator) tailored for paths and URIs.
    The collation is case-insensitive (A < a < B < b), compares numbers in a human-friendly way (9 < 10).
    Special characters such as / and @ are carefully ordered to yield a more natural ordering for import sources.

I added the base of the collation algorithm in biome_string_case because it is kind of related to character case.
Another option is to create a dedicated crate biome_string_collator or a crate biome_str_utils.

Note: This PR can be merged into main as the utilities introduced are not yet in use.

Alternative design

Instead of creating a custom collation, we could use the conventional Unicode Collation reduced to the ASCII set (biome_string_case::CldrAsciiCollator) and to parse import sources in a more advanced structure. I started with this design in mind and find it a bit too complex. However, I am still open to revisiting it if you think it could be better.

Test Plan

I added tests in the source code.

@github-actions github-actions bot added A-Core Area: core A-Linter Area: linter A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages L-CSS Language: CSS labels Oct 16, 2024
Copy link
Contributor

github-actions bot commented Oct 16, 2024

Parser conformance results on

js/262

Test result main count This PR count Difference
Total 48420 48420 0
Passed 47212 47212 0
Failed 1208 1208 0
Panics 0 0 0
Coverage 97.51% 97.51% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 40 40 0
Passed 37 37 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.50% 92.50% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 6578 6578 0
Passed 2204 2204 0
Failed 4374 4374 0
Panics 0 0 0
Coverage 33.51% 33.51% 0.00%

ts/babel

Test result main count This PR count Difference
Total 680 680 0
Passed 607 607 0
Failed 73 73 0
Panics 0 0 0
Coverage 89.26% 89.26% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 18489 18489 0
Passed 14154 14154 0
Failed 4335 4335 0
Panics 0 0 0
Coverage 76.55% 76.55% 0.00%

@Conaclos Conaclos requested review from a team October 16, 2024 15:53
@Conaclos Conaclos force-pushed the conaclos/import-sorter-revamping-source-order branch from ca34909 to 640e821 Compare October 16, 2024 16:13
Copy link

codspeed-hq bot commented Oct 16, 2024

CodSpeed Performance Report

Merging #4313 will not alter performance

Comparing conaclos/import-sorter-revamping-source-order (8cafba8) with main (30a016b)

Summary

✅ 101 untouched benchmarks

@Conaclos Conaclos force-pushed the conaclos/import-sorter-revamping-source-order branch 2 times, most recently from 8660969 to 3244afb Compare October 16, 2024 19:04
crates/biome_string_case/src/lib.rs Outdated Show resolved Hide resolved
crates/biome_string_case/src/lib.rs Outdated Show resolved Hide resolved
crates/biome_string_case/src/lib.rs Show resolved Hide resolved
crates/biome_string_case/src/lib.rs Show resolved Hide resolved
crates/biome_string_case/src/lib.rs Show resolved Hide resolved
crates/biome_string_case/src/lib.rs Show resolved Hide resolved
@Conaclos Conaclos force-pushed the conaclos/import-sorter-revamping-source-order branch 3 times, most recently from d7cea3a to 8cafba8 Compare October 17, 2024 13:41
@Conaclos Conaclos merged commit 5579f9f into main Oct 21, 2024
13 checks passed
@Conaclos Conaclos deleted the conaclos/import-sorter-revamping-source-order branch October 21, 2024 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Area: core A-Formatter Area: formatter A-Linter Area: linter L-CSS Language: CSS L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants