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_imports): support bun:, #, absolute imports #503

Merged
merged 1 commit into from
Oct 9, 2023

Conversation

Conaclos
Copy link
Member

@Conaclos Conaclos commented Oct 8, 2023

Summary

Fix #449.

I added the following categories:

  • bun:. Bunjs has recently gained a lot of traction. I think it could be great to handle bun: imports.
    I placed bun: imports just before node:. In this way we have a lexicographic order between bun, node, and npm.
  • Absolute imports such as /path.
    I placed it just after the bare specifiers (library imports).
  • Node's subpath import #import
    They are placed just after the libraries and just before the relative imports because they generally redirect to a relative path or a library.

I updated the related website page.

I noticed that we recognize implicitly exported node built-in modules as node modules. This contradicts what the documentation says: built-in Node.js modules that are explicitly imported using the node: protocol;. @arendjr what the expected behavior?

Test Plan

Updated tests.

@Conaclos Conaclos temporarily deployed to Website deployment October 8, 2023 21:59 — with GitHub Actions Inactive
@github-actions github-actions bot added A-CLI Area: CLI A-Linter Area: linter A-Parser Area: parser A-Website Area: website L-JavaScript Language: JavaScript and super languages labels Oct 8, 2023
@Conaclos Conaclos requested a review from a team October 8, 2023 21:59
@github-actions
Copy link
Contributor

github-actions bot commented Oct 8, 2023

Parser conformance results on

js/262

Test result main count This PR count Difference
Total 48863 48863 0
Passed 47808 47808 0
Failed 1055 1055 0
Panics 0 0 0
Coverage 97.84% 97.84% 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 6212 6212 0
Passed 1767 1767 0
Failed 4445 4445 0
Panics 0 0 0
Coverage 28.44% 28.44% 0.00%

ts/babel

Test result main count This PR count Difference
Total 639 639 0
Passed 571 571 0
Failed 68 68 0
Panics 0 0 0
Coverage 89.36% 89.36% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 17224 17224 0
Passed 13123 13123 0
Failed 4101 4101 0
Panics 0 0 0
Coverage 76.19% 76.19% 0.00%

@Conaclos Conaclos force-pushed the conaclos/organize-import-add-categories branch from 831e78a to 93f38b4 Compare October 8, 2023 22:25
@Conaclos Conaclos temporarily deployed to Website deployment October 8, 2023 22:25 — with GitHub Actions Inactive
Copy link
Contributor

@arendjr arendjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, thanks!

I think the documentation was slightly off here. Recognizing built-in modules and grouping them with explicit Node imports seems the right thing to do.

Given the philosophy of “grouping imports by distance” I do think it’s a bit odd to put absolute paths before URLs though. I think I would put them just before the relative imports, or the sharp imports, personally. But maybe it even depends on the use case? I don’t use absolute paths myself so I can’t have too strong an opinion 😅

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for looking after this. Here's some preliminary feedback:

  • the PR does refactor and add features, I believe the PR title should enhance the feature instead
  • it would be preferable to pull out the refactor from this PR in another PR
  • if the previous suggestion can't be done, I would update the PR description and explain the nature of the refactor. "Some code cleanup" isn't sufficient IMHO
  • absolute imports should go before relative imports and after libraries, considering that we sort them by "distance from the user"
  • Changelog line is missing

@Conaclos Conaclos force-pushed the conaclos/organize-import-add-categories branch from 93f38b4 to 8d1a6d2 Compare October 9, 2023 10:31
@Conaclos Conaclos temporarily deployed to Website deployment October 9, 2023 10:31 — with GitHub Actions Inactive
@Conaclos Conaclos force-pushed the conaclos/organize-import-add-categories branch from 8d1a6d2 to 3f2226c Compare October 9, 2023 15:21
@Conaclos Conaclos temporarily deployed to Website deployment October 9, 2023 15:21 — with GitHub Actions Inactive
@github-actions github-actions bot added A-Changelog Area: changelog and removed A-Parser Area: parser labels Oct 9, 2023
@Conaclos Conaclos force-pushed the conaclos/organize-import-add-categories branch from 3f2226c to b8e877f Compare October 9, 2023 15:23
@Conaclos Conaclos requested a review from ematipico October 9, 2023 15:23
@Conaclos Conaclos temporarily deployed to Website deployment October 9, 2023 15:24 — with GitHub Actions Inactive
@Conaclos Conaclos changed the title refactor(organize_imports): support bun:, #, absolute imports feat(organize_imports): support bun:, #, absolute imports Oct 9, 2023
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing, great addition! Thank you @Conaclos !

@ematipico ematipico temporarily deployed to Website deployment October 9, 2023 19:17 — with GitHub Actions Inactive
@Conaclos Conaclos force-pushed the conaclos/organize-import-add-categories branch from 94f71a9 to 97d29c5 Compare October 9, 2023 19:52
@Conaclos Conaclos temporarily deployed to Website deployment October 9, 2023 19:52 — with GitHub Actions Inactive
@Conaclos Conaclos merged commit 1e3fc94 into main Oct 9, 2023
17 checks passed
@Conaclos Conaclos deleted the conaclos/organize-import-add-categories branch October 9, 2023 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-CLI Area: CLI A-Linter Area: linter A-Website Area: website L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

📎 organizeImports: node > npm > absolute > relative
4 participants