rome / tools

Unified developer tools for JavaScript, TypeScript, and the web
https://docs.rome.tools/
MIT License
23.75k stars 659 forks source link

☂️ Sort imports #3462

Closed MichaReiser closed 1 year ago

MichaReiser commented 2 years ago

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

Prior work

2512

Inspiration

prettier-plugin-sort-imports

MichaReiser commented 2 years ago

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

Conaclos commented 2 years ago

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

ematipico commented 2 years ago

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 commented 2 years ago

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:

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

qweered commented 1 year ago

This may be helpful plugin-simple-import-sort

IanVS commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Let's figure out a way to prioritize this.

lcswillems commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

hyoretsu commented 1 year ago

Imo this is also an implementation to consider

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

Aaronkala commented 1 year ago

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 commented 1 year ago

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

lcswillems commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

4326 could be an alternative ti import grouping.

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