import-js / eslint-plugin-import

ESLint plugin with rules that help validate proper imports.
MIT License
5.45k stars 1.55k forks source link

Adopt ESLint core `sort-imports` rule into `import/order` #2521

Open JounQin opened 2 years ago

JounQin commented 2 years ago

The core eslint rule will support most TS usecases, but it won't support "import equals" and it won't allow different treatment of type/value imports.

That being said - there are so many different sorting rules and plugins honestly I wouldn't want to add an extension rule to ts-eslint.

There are better alternatives like import/order or simple-import-sort/imports for example.

We already made this mistake with the no-duplicate-imports rule (import/no-duplicates is a much better version, and we'll be deleting our version in the next major).

Originally posted by @bradzacher in https://github.com/import-js/eslint-plugin-import/issues/2520#issuecomment-1208750865

JounQin commented 2 years ago

cc @bradzacher @ljharb

Let's continue the discussion here.

ljharb commented 2 years ago

What features does the core rule have that we lack?

The only one I'm aware of off the top of my head is the ability to sort named imports within a single import (or possibly export) statement.

JounQin commented 2 years ago

I think the important first task will be to thoroughly audit the core rule, and figure out what the actual missing feature comparisons are.

For me personally, I only need a ignoreMemberSort option, and it should be false by default.

ljharb commented 2 years ago

I think that you mean the reverse? a enforceMemberSort option that's false by default?

JounQin commented 2 years ago

I think that you mean the reverse? a enforceMemberSort option that's false by default?

That would be great, I just want to point out how ESLint core handle this in sort-imports rule.

JounQin commented 2 years ago

@ljharb Are we in agreement? (Is this change accepted? I'd like to work on this, if so.)

ljharb commented 2 years ago

I'd first like to understand what are all the feature gaps - if we're trying to replace the core rule, then we'd need to account for all of its use cases.

JounQin commented 2 years ago

I'd first like to understand what are all the feature gaps - if we're trying to replace the core rule, then we'd need to account for all of its use cases.

Did you mean https://eslint.org/docs/latest/rules/sort-imports#options?

ljharb commented 2 years ago

Yes, and in particular, the use cases provided for by the various combinations of options.

JounQin commented 2 years ago

Yes, and in particular, the use cases provided for by the various combinations of options.

So I'm still not for sure what's the next action to do? Maybe what options should be provided?

ehoogeveen-medweb commented 2 years ago

One thing I would not like to see is for import/order to adopt the behavior of sort-imports where sorting by member syntax appears to be mandatory and memberSyntaxSortOrder, if provided, must be an array containing all four values. I don't understand why enforcing a specific ordering between imports with a single named import and imports with multiple named imports is desirable, much less mandatory.

Aside from the mentioned ignoreMemberSort, I guess the other option that sort-imports has that import/order does not is ignoreDeclarationSort: import/order lets you sort by import path, but not by import name (though personally I'm not sure sorting by import name makes much sense).

Edit: Thinking about it a bit more, I suppose that

So there really isn't a lot of overlap between the two, but sorting names inside a named import would be a nice enhancement.

ljharb commented 2 years ago

The next action is "research" :-) someone needs to do the work to figure out an exhaustive list of what is possible with the core rule, and with the import rule, and then figure out how we can close the gap.

@ehoogeveen-medweb yes, I definitely agree that it's better that we're more flexible.

The main thing is, that if we provide a migration path for every user of sort-imports, then eslint will likely deprecate the rule and point people to us instead - that seems like a useful goal.

NishargShah commented 2 years ago

One thing I would not like to see is for import/order to adopt the behavior of sort-imports where sorting by member syntax appears to be mandatory and memberSyntaxSortOrder, if provided, must be an array containing all four values. I don't understand why enforcing a specific ordering between imports with a single named import and imports with multiple named imports is desirable, much less mandatory.

So there really isn't a lot of overlap between the two, but sorting names inside a named import would be a nice `enhancement.

Yes adding a member sort alphabetically is a very good choice.

There is also one good functionality in sort-imports, which allows you to specify memberSyntaxSortOrder, and sometimes it is mandatory because it looks good if all the multiple members are in the bottom ( sort by import path again ).

Currently, I can not use memberSyntaxSortOrder because it conflicts ( import is using a path where sort-imports uses a name )

EDIT

Adding ignoreDeclarationSort: true automatically sorts the members of the import without giving any extra error.

"sort-imports": [
  "error",
  {
    "ignoreDeclarationSort": true
  }
],
"import/order": [
  "error",
  {
    "groups": [
      "builtin",
      "external",
      "internal",
      ["parent", "sibling", "index"],
      "object",
      "type"
    ],
    "newlines-between": "always",
    "alphabetize": {
      "order": "asc",
      "caseInsensitive": true
    }
  }
],
unional commented 10 months ago

The issue I observed is that when import/order perform auto fix, the named import are sorted with type comes first. e.g.:

import { type $Else,type $Then, type IsBigint, testType } from '../index.js'

But VSCode organize import will sort them with type comes last:

import { testType, type $Else, type $Then, type IsBigint } from '../index.js'

It would be great if they are consistent.