Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

☂️ Sort imports #3462

Closed
4 of 5 tasks
MichaReiser opened this issue Oct 20, 2022 · 20 comments · Fixed by #3818
Closed
4 of 5 tasks

☂️ Sort imports #3462

MichaReiser opened this issue Oct 20, 2022 · 20 comments · Fixed by #3818
Assignees
Labels
A-Formatter Area: formatter umbrella Issue to track a collection of other issues
Milestone

Comments

@MichaReiser
Copy link
Contributor

MichaReiser commented Oct 20, 2022

Description

Today's editors have very good support for automatically adding imports but do so by adding them at the end of the import statements (or specifiers).

The result is that it is often necessary to manually tune the imports to, e.g. group them by source.

Goal

Add support for sorting imports and import specifiers to the formatter. Sorting imports should be opt-in because changing the ordering of import statements changes the order in which imported modules are executed, resulting in observable differences.

Tasks

  • Add a new option to enable import sorting (disabled by default)
  • Sort (natural ordering) import specifiers (per import statement, not across import statements)
  • Sort (natural ordering) import statements by their module source. Treat an empty line or non-import statement as the start of a new import statement group and do the sorting per group.
  • Add configuration support to automatically form groups depending on the module source.
  • CLI integration

Prior work

#2512

Inspiration

prettier-plugin-sort-imports

@MichaReiser MichaReiser added umbrella Issue to track a collection of other issues A-Formatter Area: formatter labels Oct 20, 2022
@MichaReiser
Copy link
Contributor Author

@leops I remember we once discussed whether this is something that should be implemented in the formatter or as a lint rule due to its unsafe nature. Do you remember our conclusion?

@MichaReiser MichaReiser changed the title ☂️ 📎 Formatter: Sort imports ☂️ Formatter: Sort imports Oct 20, 2022
@Conaclos
Copy link
Contributor

IMO this should be a linting rule with auto fix. I never heard about import ordering at formatter level…

@ematipico
Copy link
Contributor

ematipico commented Oct 20, 2022

IMO this should be a linting rule with auto fix. I never heard about import ordering at formatter level…

There's a plugin for prettier, Micha attached a link to it and the end of the description :) There are other languages that do formatting import, for example in Rust. The issue is that in JavaScript, we have side effects, so the ordering of imports (at any feature, formatter or linter) is not safe.

But if someone is writing a library and their code is side effect-free, ordering imports is safe.

@MichaReiser
Copy link
Contributor Author

MichaReiser commented Oct 20, 2022

IMO this should be a linting rule with auto fix. I never heard about import ordering at formatter level…

Sorting imports is a tricky one, and I can see reasons for it to be implemented in the formatter OR linter because I think it mainly comes down to the user's preference of when to sort imports:

  • On save
  • Only when manually applied

I don't see a reason why it shouldn't be possible to add support for code actions that are automatically applied when saving a file if the user explicitly opted in for that.

The main reason why I would still implement the functionality as part of the formatter at the time is that the formatter has superior comments support that ensures that no comments are dropped, which I see as essential for any functionality that automatically runs on save.

We may decide in the future to move the logic from the formatter to the linter or to also offer a lint rule where the code fix calls into the formatter.

Other prior art when it comes to sorting "imports" is

  • Rustfmt sorts 'use' statements PR
  • IntelliJ or Eclipse Stackoverflow. Supported for Java, JavaScript and potentially other languages.

@qweered
Copy link

qweered commented Oct 24, 2022

This may be helpful plugin-simple-import-sort

@MichaReiser MichaReiser changed the title ☂️ Formatter: Sort imports ☂️ Sort imports Oct 28, 2022
@MichaReiser MichaReiser added this to the Next milestone Oct 28, 2022
@IanVS
Copy link
Contributor

IanVS commented Nov 9, 2022

I maintain a fork of the prettier plugin mentioned above which respects side-effect imports because it is unsafe to sort them. https://www.npmjs.com/package/@ianvs/prettier-plugin-sort-imports

Having some kind of functionality to sort imports similar to that plugin is the biggest thing that will keep me from adopting rome. Otherwise, the speed is amazing and I'd jump on it in a heartbeat.

@lcswillems
Copy link

I am currently using Prettier with the sort imports plugins. With Rome 10 announcement I was eager to replace Prettier by Rome but the lack of imports sorting is a no-go for me for the moment.

@jeysal
Copy link
Contributor

jeysal commented Nov 10, 2022

This might be a bit far into the future, but a lint rule could evolve to do cool stuff like

  1. Check package.json for sideEffects: false
  2. If it's set, offer reordering as a safe fix. Else, offer reordering and writing sideEffects: false into package.json as a suggested fix with a good diagnostic explaining the implication.

or something along those lines.

Whether sideEffects is even a good convention that Rome would want to support, is of course a different question.
But either way, this seems to me like something that extends far beyond the formatter. I also think there's a whole class of features hidden behind this, such as linting side-effect imports or obviously side-effect-full statements in code declared to be side-effect free.

I would thus argue against constraining Rome in how these things will be handled in the future by putting something like this into the formatter, which is supposed to operate on a plain AST input and a few config options with no further project context.

@sebmck
Copy link
Contributor

sebmck commented Nov 13, 2022

Let's figure out a way to prioritize this.

@lcswillems
Copy link

Thanks to this issue and the comment of @IanVS, I have discovered his plugin:

https://github.com/ianvs/prettier-plugin-sort-imports

His plugin doesn't sort imports with side effects and add great features such as importOrderMergeDuplicateImports. Great work Ian!

I hope one day this will be built-in in Rome to not have to install an additional plugin and enjoy fast formatting / linting. For the moment I am staying with Prettier.

@lcswillems
Copy link

@leops I see that PR closed this issue. Does the PR implement the last checkbox of the issue, i.e. "Add configuration support to automatically form groups depending on the module source."?

If not, do you want me to open another issue for this particular point? This is the remaining point preventing me to use Rome.

@leops
Copy link
Contributor

leops commented Nov 25, 2022

Sorry I forgot I had tagged this issue in the PR, it got erroneously closed since #3818 only implements the baseline feature, but it's still missing many crucial features (and in particular everything related to configuration)

@lcswillems
Copy link

Hi @MichaReiser ,

What do you mean by "Add configuration support to automatically form groups depending on the module source."? Do you mean grouping with regex such that prettier-plugin-sort-imports allows to do?

@lcswillems
Copy link

lcswillems commented Dec 12, 2022

In my case, I use the prettier-plugin-sort-imports plugin of ianvs.

And this configuration in my package.json:

"prettier": {
  "importOrder": [
    "^~(.*)$",
    "^\\$(.*)$",
    "^[./]"
  ],
  "importOrderBuiltinModulesToTop": true,
  "importOrderMergeDuplicateImports": true,
  "importOrderCombineTypeAndValueImports": true
},

The most important setting is "importOrder". The other ones are less important.

@ematipico ematipico modified the milestones: 11.0.0, Next Dec 12, 2022
@hyoretsu
Copy link

hyoretsu commented Jan 3, 2023

Imo this is also an implementation to consider

https://github.com/Tibfib/eslint-plugin-import-helpers

@Aaronkala
Copy link

This is a popular eslint version https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/order.md. We have eslint fix all setup to run on save after prettier.

@ematipico
Copy link
Contributor

This feature will be shipped in the next release: v12.0.0

@lcswillems
Copy link

@ematipico Has the point "Add configuration support to automatically form groups depending on the module source." been added? I don't see configuration to group imports.

@ematipico
Copy link
Contributor

@ematipico Has the point "Add configuration support to automatically form groups depending on the module source." been added? I don't see configuration to group imports.

No, the scope of feature has been drastically reduced due to reduced manpower.

It might be implemented later on based on how stable the feature is.

I would suggest to create a discussion to propose how a possible configuration could look like.

@Conaclos
Copy link
Contributor

Conaclos commented Mar 29, 2023

#4326 could be an alternative ti import grouping.

By the way, I could prefer to have import grouping without any extra configuration.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Formatter Area: formatter umbrella Issue to track a collection of other issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.