tools icon indicating copy to clipboard operation
tools copied to clipboard

☂️ Sort imports

Open MichaReiser opened this issue 3 years ago • 13 comments

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

  • [x] Add a new option to enable import sorting (disabled by default)
  • [x] Sort (natural ordering) import specifiers (per import statement, not across import statements)
  • [x] 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 avatar Oct 20 '22 09:10 MichaReiser

@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 avatar Oct 20 '22 09:10 MichaReiser

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

Conaclos avatar Oct 20 '22 10:10 Conaclos

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.

ematipico avatar Oct 20 '22 10:10 ematipico

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.

MichaReiser avatar Oct 20 '22 12:10 MichaReiser

This may be helpful plugin-simple-import-sort

qweered avatar Oct 24 '22 09:10 qweered

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.

IanVS avatar Nov 09 '22 21:11 IanVS

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.

lcswillems avatar Nov 10 '22 12:11 lcswillems

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.

jeysal avatar Nov 10 '22 13:11 jeysal

Let's figure out a way to prioritize this.

sebmck avatar Nov 13 '22 19:11 sebmck

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 avatar Nov 20 '22 21:11 lcswillems

@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.

lcswillems avatar Nov 25 '22 15:11 lcswillems

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)

leops avatar Nov 25 '22 16:11 leops

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 avatar Dec 10 '22 16:12 lcswillems

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.

lcswillems avatar Dec 12 '22 09:12 lcswillems

Imo this is also an implementation to consider

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

hyoretsu avatar Jan 03 '23 04:01 hyoretsu

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.

Aaronkala avatar Mar 21 '23 18:03 Aaronkala

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

ematipico avatar Mar 21 '23 18:03 ematipico

@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.

lcswillems avatar Mar 29 '23 07:03 lcswillems

@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.

ematipico avatar Mar 29 '23 07:03 ematipico

#4326 could be an alternative ti import grouping.

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

Conaclos avatar Mar 29 '23 10:03 Conaclos