-
-
Notifications
You must be signed in to change notification settings - Fork 548
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(assist): import organizer revamping #5364
base: main
Are you sure you want to change the base?
Conversation
55e0cba
to
2891850
Compare
Parser conformance results onjs/262
jsx/babel
symbols/microsoft
ts/babel
ts/microsoft
|
091d124
to
8e0ab47
Compare
8e0ab47
to
965c65f
Compare
CodSpeed Performance ReportMerging #5364 will not alter performanceComparing Summary
|
965c65f
to
0a39ae9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a full review, looks great at a glance!
0a39ae9
to
3274c15
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Do you still want to include this into the beta?
...cases_overrides_organize_imports/does_handle_included_file_and_disable_organize_imports.snap
Outdated
Show resolved
Hide resolved
crates/biome_cli/tests/snapshots/main_commands_ci/ci_formatter_linter_organize_imports.snap
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/assist/source/organize_imports/util.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/assist/source/organize_imports/util.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/assist/source/organize_imports/import_source.rs
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/src/assist/source/organize_imports/import_source.rs
Show resolved
Hide resolved
Yes. I can add something to the blog post. |
261b2f0
to
ea01bdd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation must be updated to reflect the new options. Which means that:
- we need to document
legacy
and how it works (probably not needed but it needs to be highlighted) - we need to document
importGroups
and provide enough examples to explain the new behaviour
I'm not going to block the PR, but this is something we must do before the beta, if we plan to release it for the beta
506f5a6
to
945cdd6
Compare
945cdd6
to
ab1dbae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we’re getting there, right? 😊
I have just committed the blank-line between groups feature. I have to document the new system before merging the PR. |
0f6a3e6
to
871bdb3
Compare
dffa3cc
to
b92d845
Compare
The documentation is committed. Feel free to review. |
b92d845
to
6d0760b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an outstanding feature; thank you, @Conaclos! I'm sure a lot of users will be pleased. I hope you're proud of what you just did.
I left various comments around the docs. Many are grammar fixes. Here are some high-level pointers:
- consider a glossary to explain specific terms to place at the beginning
- Let's first discuss our defaults without mentioning custom groups. Then, once all the important information is listed, we can have "custom groups" at the very end. A user needs to know about default groups, blank lines, comments, etc., to create groups.
- Before showing the example, could you explain it and describe its outcome, too? I know the codegen shows a diagnostic, but it's important for accessibility too.
/// Chunks also ends as soon as a statement or a side-effect import (also called bare import) is encountered. | ||
/// The following example contains three chunks. | ||
/// | ||
/// ```js,ignore | ||
/// import A from "a"; | ||
/// import "x"; | ||
/// import * as B from "b"; | ||
/// function f() {} | ||
/// import * as C from "c"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Chunks also ends as soon as a statement or a side-effect import (also called bare import) is encountered. | |
/// The following example contains three chunks. | |
/// | |
/// ```js,ignore | |
/// import A from "a"; | |
/// import "x"; | |
/// import * as B from "b"; | |
/// function f() {} | |
/// import * as C from "c"; | |
/// Chunks also end when a statement or a side-effect import (or bare import) is encountered. | |
/// The following example contains three chunks. | |
/// | |
/// ```js,ignore | |
/// import A from "a"; | |
/// import "x"; | |
/// import * as B from "b"; | |
/// function f() {} | |
/// import * as C from "c"; |
In this example, it isn't how the chunks are divided; I suggest explaining them. For instance, I don't understand if import "x"
belongs to a chunk or not, and if it does, to which chunk belongs to.
Also, I suggest adding some exports so we keep the examples consistent.
/// The import and export organizer ensures that chunks are separated from each other with a blank lines. | ||
/// Only side-effect imports adjacent to a chunk are not separated by a blank line. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// The import and export organizer ensures that chunks are separated from each other with a blank lines. | |
/// Only side-effect imports adjacent to a chunk are not separated by a blank line. | |
/// The action ensures that chunks are separated from each other with blank lines. | |
/// Only side-effect imports adjacent to a chunk are not separated by a blank line. |
We should start using the word "action". This is an action/rule like all the others.
Also, regarding the second line, maybe we should provide an example.
/// | ||
/// [Side effect imports](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import#forms_of_import_declarations) are import statements that usually don't import any name: | ||
/// The source of an import or export is the string coming just after the `from` keyword. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We used the word "source" many times , and the explanation comes only at this point of the docs. We should explain it at the very beginning.
Also, I know you decided to use the word "source" because it might be more beginner-friendly, but I wonder if we should at least mention that it is technically called specifier.
/// ## Comment handling | ||
/// | ||
/// When sorting imports and exports, attached comments are moved with their import or export, | ||
/// and detached comments (comments followed by a blank line) are left where they are. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This explanation goes in contrast with the previous explanation. The previous explanation said that a detached comment needs to be surrounded by blank lines. Or maybe the previous explanation, with the example, confused me.
/// Note that there is an exception at the rule. | ||
/// If a comment appears at the top of the file, it is considered as detached even if no blank lien follows. | ||
/// This ensures that copyright notice and file header comment stay at the top of the file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Note that there is an exception at the rule. | |
/// If a comment appears at the top of the file, it is considered as detached even if no blank lien follows. | |
/// This ensures that copyright notice and file header comment stay at the top of the file. | |
/// However, there is an exception to the rule. | |
/// If a comment appears at the top of the file, it is considered detached even if no blank line follows. | |
/// This ensures that copyright notice and file header comments stay at the top of the file. |
Be careful!! The first example that talked about attached comments had a comment at the top of the file, which is considered an attached comment 😄
/// This ensures that copyright notice and file header comment stay at the top of the file. | ||
/// | ||
/// ```js,expect_diagnostic | ||
/// // Copyright notice and file header comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that I am reviewing only the docs now, but I wonder if I should narrow down the expectation only to multiline comments. What do you think?
6d0760b
to
5f535ae
Compare
Summary
This long awaited feature is finally here! It is one of the most difficult features I have implemented. It took me one failed attempt and many hours. I am very happy with the result.
This PR implements #3177 with some divergences:
This is left to a future PR.
This drastically simplifies import matching and makes it more powerful.
This new import organizer provides the following features:
It orders named specifiers using a natural order
It orders import attributes according to their keys using a natural order
Imports and exports are separated into chunks before to be sorted.
A new chunk starts every time we met a side effect (bare) import, a distinct statement kind, or a detached comment.
A detached comment is separated by a blank line of the import/export.
Chunk of imports or exports are first sorted according to the group they belong to.
By default, there is a single group: the implicit group.
Users can define their own groups and define an order between them.
The implicit group always comes last.
Groups are either predefined (like
:BUN:
and:NODE:
) or globs with exceptions.Imports of a group are sorted according to other parameters, notably their import source.
See the RFC and the implementation for more details.
The implementation takes care of removing and adding newlines where required.
Chunks are separated by a blank line except for side effect imports when other imports surround them.
Comments are either attached or detached to an import/export.
A detached comment (separated by a blank line of the import/export) creates a new chunk and is kept above the chunk.
I added an exception for the header comment in a file: this comment is never attached. This ensures that Copyright notice are never moved.
Limitation / Possible improvements
@scope/lib
doesn't match the glob@scope/lib/**
.This could create some frictions.
node:**
This could be easily fixed.
As a workaround, users have to match against both
node:*
andnode:*/**
Trivia handling when sorting named specifiers and attributes should be improved.(Done)Some data structures could be shared for sorting distinct named specifiers and import attributes.(Not worth the complexity?)Implementation strategy
The
run
procedure checks if everything is organized and register any "issues" that it founds.An issue can report an unorganized import/export or an unsorted chunk (more precisely unsorted prefix of a chunk).
The
action
procedure is responsible for fixing the found issues.This is different of the previous implementation where the entire sorting logic was in
run
.This new approach pursuits two objectives:
Notably all the chunk logic is in
run
.action
doesn't bother about it.Also, by dividing chunk sorting and individual import/export organizer, we are able to avoid a costly chunk sort when only named specifiers of an import are unsorted.
I think there is room for improving the code. However, it is already a big PR and I would like to ship it before we release Biome 2.0 beta.
Test Plan
I added some tests.