import-js / eslint-plugin-import

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

feat(import/order): enable advanced spacing and sorting of type-only imports #3104

Open Xunnamius opened 3 days ago

Xunnamius commented 3 days ago

Resolves #2441, #2615, #2347, #2172 \ Related to #2912

First, thanks for making eslint-plugin-import! It has saved me a lot of time and headache.

This PR implements in import/order (1) intragroup sorting of type-only imports (to match intergroup sorting of normal imports), (2) newline controls for type-only imports, and (3) a new option to consolidate newlines and save space where possible.

Along the way, I also encountered a bug where a NaN rank would get passed around in some edge cases (such as using an import with an absolute specifier under certain configurations). I fixed it (applied to computeRank) and added a test to catch it, both of which are included in this PR.

The documentation updates associated with the changes in this PR are part of a separate PR since those changes probably warrant further discussion. This PR also contains additional tests that cover all examples in the documentation, including for the other settings; several of the old "passing" rule examples failed and were corrected.

If this PR is too large (most of the additions are in the test files), I still have access to the atomic commits and can split the features up in whatever way is easiest. Otherwise, this PR contains the implementation and tests for the following:

Allow intragroup sorting of type-only imports

This is implemented via sortTypesAmongThemselves. The proposed documentation corresponding to this new feature can be previewed here.

Example Given this code (which is already correctly ordered): ```typescript import type A from 'fs'; import type B from 'path'; import type C from '../foo.js'; import type D from './bar.js'; import type E from './'; import a from 'fs'; import b from 'path'; import c from '../foo.js'; import d from './bar.js'; import e from './'; ``` And the following settings, the rule check will fail: ```jsonc { "groups": ["type", "builtin", "parent", "sibling", "index"], "alphabetize": { "order": "asc" } } ``` With `--fix` yielding: ```typescript import type C from '../foo.js'; // ??? import type A from 'fs'; import type B from 'path'; import type D from './bar.js'; import type E from './'; import a from 'fs'; import b from 'path'; import c from '../foo.js'; import d from './bar.js'; import e from './'; ``` This is currently the technically correct behavior, but the wrong outcome in my opinion. However, with the following settings, the rule check will succeed instead: ```diff { "groups": ["type", "builtin", "parent", "sibling", "index"], "alphabetize": { "order": "asc" }, + "sortTypesAmongThemselves": true } ``` Of course, achieving the reverse order (type-only imports after normal imports) is possible by moving `"type"` to the end of `groups`: ```diff { + "groups": ["builtin", "parent", "sibling", "index", "type"], "alphabetize": { "order": "asc" }, "sortTypesAmongThemselves": true } ```

It was only after implementing this feature, when I started reviewing the issues literature, that I saw #2441, which lead me to eslint-plugin-perfectionist (neat) and its implementation of "builtin-type," "external-type," etc. I much prefer this PR's implementation, which takes a similar approach to this comment. I don't see how allowing type-only builtin/external/etc imports to be sorted in a different order than non-type builtin/external/etc imports makes things less confusing. And that's before we consider custom pathGroups.

Instead, the goal of sortTypesAmongThemselves is to allow the "type" group to be sorted among itself in a backward-compatible way without the added complexity (and potential inconsistency) of adding regexp or "*-type" groups to groups. I'm already writing too much configuration. I just want to flick a switch 😅.

Additionally, the code that makes sortTypesAmongThemselves work could be extended to make something like #2912 work too (perhaps through another option), though that use case is not of interest to me currently.

Allow controlling intragroup spacing of type-only imports

This is implemented via newlines-between-types. The proposed documentation corresponding to this new feature can be previewed here.

Example Given this code: ```typescript import type A from 'fs'; import type B from 'path'; import type C from '../foo.js'; import type D from './bar.js'; import type E from './'; import a from 'fs'; import b from 'path'; import c from '../foo.js'; import d from './bar.js'; import e from './'; ``` And the following settings, the rule check will fail: ```jsonc { "groups": ["type", "builtin", "parent", "sibling", "index"], "sortTypesAmongThemselves": true, "newlines-between": "always" } ``` With `--fix` yielding: ```typescript import type A from 'fs'; import type B from 'path'; import type C from '../foo.js'; import type D from './bar.js'; import type E from './'; import a from 'fs'; import b from 'path'; import c from '../foo.js'; import d from './bar.js'; import e from './'; ``` However, with the following settings, the rule check will succeed instead: ```diff { "groups": ["type", "builtin", "parent", "sibling", "index"], "sortTypesAmongThemselves": true, "newlines-between": "always", + "newlines-between-types": "ignore" } ```

sortTypesAmongThemselves allows sorting type-only and normal imports separately. By default, newlines-between will govern all newlines between import statements like normal.

I generally want my type-only imports to be sorted for ease of reference but never have newlines between them (save space) while I want my normal imports (which I tend to visually peruse more often) to be aesthetically pleasing, grouped, and sorted. newlines-between is too coarse-grained for this, so this PR introduces newlines-between-types, a setting identical to newlines-between except it only applies to type-only imports, and only when sortTypesAmongThemselves is enabled (i.e. it is backward-compatible).

When newlines-between and newlines-between-types conflict, newlines-between-types takes precedence for type-only imports. For normal imports, newlines-between-types is ignored entirely.

One issue that might warrant further discussion is which setting governs the newline separating type-only imports from normal imports. Right now, I have it so newlines-between-types controls this space, but perhaps it should be its own setting.

Collapse excess spacing for aesthetically pleasing imports

This is implemented via consolidateIslands. The proposed documentation corresponding to this new feature can be previewed here.

Example Given this code (which could be the output of a previous `--fix` pass): ```typescript var fs = require('fs'); var path = require('path'); var { util1, util2, util3 } = require('util'); var async = require('async'); // Ugly but technically valid var relParent1 = require('../foo'); var { relParent21, relParent22, relParent23, relParent24, } = require('../'); var relParent3 = require('../bar'); var { sibling1, sibling2, sibling3 } = require('./foo'); var sibling2 = require('./bar'); var sibling3 = require('./foobar'); ``` And the following settings, the rule check will _pass_: ```jsonc { "newlines-between": "always-and-inside-groups" } ``` However, when given the following instead, the rule check will _fail_: ```diff { "newlines-between": "always-and-inside-groups", + "consolidateIslands": "inside-groups" } ``` With `--fix` yielding: ```typescript var fs = require('fs'); var path = require('path'); var { util1, util2, util3 } = require('util'); var async = require('async'); // Pretty var relParent1 = require('../foo'); var { relParent21, relParent22, relParent23, relParent24, } = require('../'); var relParent3 = require('../bar'); var { sibling1, sibling2, sibling3 } = require('./foo'); var sibling2 = require('./bar'); var sibling3 = require('./foobar'); ``` Note how the intragroup "islands" of grouped single-line imports, as well as multi-line imports, are surrounded by new lines.

Essentially, I was looking for a newlines-between-like setting somewhere between "never" and "always-and-inside-groups". I want newlines separating groups/pathGroups imports from one another (like "always-and-inside-groups"), newlines separating imports that span multiple lines from other imports (this is the new thing), and any remaining newlines deleted or "consolidated" (like "never"). The example above demonstrates this use case.

Right now, this is achievable with newlines-between set to "always-and-inside-groups" if you add additional newlines around multi-line imports to every file by hand. The goal of consolidateIslands is to allow eslint --fix to take care of the tedium in a backward-compatible way.

There was a slight complication with consolidateIslands though: while testing across a few mid-sized repos, I discovered my naive implementation caused a conflict when enabled alongside sortTypesAmongThemselves: true, newlines-between: "always-and-inside-groups", and newlines-between-types: "never"... and then only when a normal import was followed by a multi-line type-only import. This conflict makes sense, since newlines-between-types: "never" wants no newlines ever and demands no newline separate type-only imports from normal imports (since, currently, newlines-between-types governs that space), yet consolidateIslands demands a newline separate all multi-line imports from other imports.

To solve this, the current implementation has newlines-between-types yield to consolidateIslands whenever they conflict. I've also added a test to catch any regressions around this edge case.


A demo package containing these features is available in the registry for easy testing:

npm install --save-dev eslint-plugin-import@npm:@-xun/eslint-plugin-import-experimental
codecov[bot] commented 3 days ago

Codecov Report

Attention: Patch coverage is 98.64865% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.16%. Comparing base (a20d843) to head (c5a0944).

Files with missing lines Patch % Lines
src/rules/order.js 98.64% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #3104 +/- ## ========================================== - Coverage 95.28% 95.16% -0.13% ========================================== Files 83 83 Lines 3584 3637 +53 Branches 1252 1295 +43 ========================================== + Hits 3415 3461 +46 - Misses 169 176 +7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.


🚨 Try these New Features: