Skip to content

feat: add option followTsOrganizeImports for order rule #287

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 22 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
77e6cd3
fix: ts organize imports mismatch for hashtag imports
Shinigami92 Apr 10, 2025
12c17a9
refactor: rename mapped to private-imports
Shinigami92 Apr 11, 2025
31854ca
refactor: rename `private-imports` to `private-import`
JounQin Apr 11, 2025
dd1f012
chore: add privateImportsFeatureFlag option to order rule
Shinigami92 Apr 11, 2025
7b42ee0
chore: enhance
Shinigami92 Apr 11, 2025
2790318
chore: enhance
Shinigami92 Apr 11, 2025
9a0edb7
chore: rename to just private
Shinigami92 Apr 14, 2025
e47f0ad
chore: add warning
Shinigami92 Apr 14, 2025
f2b6011
test: add explicit followTsOrganizeImports=false case
Shinigami92 Apr 14, 2025
605a9f8
test: groups take precedence over followTsOrganizeImports
Shinigami92 Apr 14, 2025
708b5cc
chore: improve warning message
Shinigami92 Apr 14, 2025
3ea0b5b
chore: use internal log
Shinigami92 Apr 14, 2025
6a1397f
chore: add changeset
Shinigami92 Apr 14, 2025
213eb21
docs: update
Shinigami92 Apr 14, 2025
923ffb4
chore: remove unnecessary log
Shinigami92 Apr 14, 2025
48fc805
chore: test for private as last
Shinigami92 Apr 15, 2025
3eaeedc
test: add TypeScript path mapping tests
Shinigami92 Apr 15, 2025
a80faf7
chore: use testFilePath
Shinigami92 Apr 15, 2025
e4ff449
test: add mapped data
Shinigami92 Apr 16, 2025
f5ab8e8
Merge branch 'master' into fix-ts-organize-imports-mismatch-for-hasht…
Shinigami92 Apr 16, 2025
e9d728b
test: add external looking import case
Shinigami92 Apr 16, 2025
ab4ec1d
test: add case with separate group
Shinigami92 Apr 16, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/clear-bikes-jog.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"eslint-plugin-import-x": minor
---

feat: add option `followTsOrganizeImports` for `order` rule
14 changes: 14 additions & 0 deletions docs/rules/order.md
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ This rule supports the following options (none of which are required):
- [`sortTypesGroup`][7]
- [`newlines-between-types`][27]
- [`consolidateIslands`][25]
- [`followTsOrganizeImports`][26]

---

Expand Down Expand Up @@ -958,6 +959,17 @@ import type { G } from './aaa.js'
import type { H } from './bbb'
```

### `followTsOrganizeImports`

Valid values: `boolean` \
Default: `false`

> [!CAUTION]
>
> Currently, `followTsOrganizeImports` defaults to `false`. However, in a later update, the default might change to `true`.

When set to `true`, this option will align the behavior with [TypeScript's LSP Organize Imports][34] feature. This only has an effect if no manual `groups` are defined.

## Related

- [`import-x/external-module-folders`][29]
Expand All @@ -984,10 +996,12 @@ import type { H } from './bbb'
[22]: https://prettier.io
[23]: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-5.html#type-modifiers-on-import-names
[25]: #consolidateislands
[26]: #followtsorganizeimports
[27]: #newlines-between-types
[28]: ../../README.md#importinternal-regex
[29]: ../../README.md#importexternal-module-folders
[30]: #alphabetize
[31]: https://webpack.js.org/guides/tree-shaking#mark-the-file-as-side-effect-free
[32]: #distinctgroup
[33]: #named
[34]: https://code.visualstudio.com/docs/typescript/typescript-refactoring#_organize-imports
29 changes: 28 additions & 1 deletion src/rules/order.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,15 @@ const defaultGroups = [
'index',
] as const

const defaultGroupsTsOrganizeImports = [
'private',
'external',
'builtin',
'parent',
'index',
'sibling',
] as const
Comment on lines +56 to +63
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order in defaultGroupsTsOrganizeImports appears to be inconsistent with TypeScript's LSP Organize Imports behavior. Based on the tests and documentation, TypeScript organizes imports with 'builtin' modules before 'external' modules, but this implementation has 'external' before 'builtin'. Consider swapping these two entries to match TypeScript's actual ordering behavior.

Suggested change
const defaultGroupsTsOrganizeImports = [
'private',
'external',
'builtin',
'parent',
'index',
'sibling',
] as const
const defaultGroupsTsOrganizeImports = [
'private',
'builtin',
'external',
'parent',
'index',
'sibling',
] as const

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Shinigami92 Can you double check?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think builtin should be front of external.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll check, but my gut feeling tells me either we missed a import-statement case, or it works correctly right now already and the AI is hallucinating

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AH! so an external import is something like e.g. glob 👍
Will add to test

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to get a test case for separated node:* with followTsOrganizeImports: true?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, you can still have all your node:* imports at the top if you but it in a separate group (split them with a newline from the other imports)

Wouldn't the group order be different?

Do you want to get a test case for separated node:* with followTsOrganizeImports: true?

Thta's would be appreciated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't the group order be different?

No, why should it?

That's would be appreciated.

Added ab4ec1d (#287)

You should take a case like I added here, and copy-paste it into your VSCode and then hit CTRL+SHIFT+P and run organize imports, then you can experiment yourself a bit with it 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally we got failing test cases, I don't know whether it's expected to you.

Copy link
Member

@JounQin JounQin Apr 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, why should it?

Didn't the default order external should be listed in front of builtin as you set? Did I miss anything?

You should take a case like I added here, and copy-paste it into your VSCode and then hit CTRL+SHIFT+P and run organize imports, then you can experiment yourself a bit with it 🙂

I also use this command a lot when I develop projects not using this plugin nor simple-sort-imports, etc, for example: https://github.com/backstage/backstage.


// REPORTING AND FIXING

function reverse(array: ImportEntryWithRank[]): ImportEntryWithRank[] {
Expand Down Expand Up @@ -824,6 +833,7 @@ function getRequireBlock(node: TSESTree.Node) {
const types: ImportType[] = [
'builtin',
'external',
'private',
'internal',
'unknown',
'parent',
Expand Down Expand Up @@ -1149,6 +1159,7 @@ export interface Options {
pathGroups?: PathGroup[]
sortTypesGroup?: boolean
warnOnUnassignedImports?: boolean
followTsOrganizeImports?: boolean
}

type MessageId =
Expand Down Expand Up @@ -1271,6 +1282,11 @@ export default createRule<[Options?], MessageId>({
type: 'boolean',
default: false,
},
followTsOrganizeImports: {
type: 'boolean',
// TODO: switch default to true in next major
default: false,
},
},
additionalProperties: false,
dependencies: {
Expand Down Expand Up @@ -1388,8 +1404,19 @@ export default createRule<[Options?], MessageId>({
const { pathGroups, maxPosition } = convertPathGroupsForRanks(
options.pathGroups || [],
)
if (options.followTsOrganizeImports && options.groups) {
// TODO: remove warning when default of followTsOrganizeImports switched to true
// because then the user potentially knows what they are doing
// and the warning is not needed anymore
console.warn(
'If you have defined your own `groups` in `order`, `followTsOrganizeImports: true` has no effect.',
)
}
const { groups, omittedTypes } = convertGroupsToRanks(
options.groups || defaultGroups,
options.groups ||
(options.followTsOrganizeImports
? defaultGroupsTsOrganizeImports
: defaultGroups),
)
ranks = {
groups,
Expand Down
8 changes: 8 additions & 0 deletions src/utils/import-type.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,11 @@ export function isScopedMain(name: string) {
return !!name && scopedMainRegExp.test(name)
}

function isPrivate(name: string) {
// see https://nodejs.org/api/packages.html#imports
return name.startsWith('#')
}

function isRelativeToParent(name: string) {
return /^\.\.$|^\.\.[/\\]/.test(name)
}
Expand Down Expand Up @@ -158,6 +163,9 @@ function typeTest(
if (isBuiltIn(name, settings, path)) {
return 'builtin'
}
if (isPrivate(name)) {
return 'private'
}
if (isRelativeToParent(name)) {
return 'parent'
}
Expand Down
82 changes: 82 additions & 0 deletions test/rules/order.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4809,6 +4809,64 @@ describe('TypeScript', () => {
},
],
}),
// By default it should order same as TS LSP (issue-286)
tValid({
code: `import { internA } from "#a";
import { scopeA } from "@a/a";
import a from 'a';
import 'format.css';
import fs from 'node:fs';
import path from "path";
import index from './';
import { localA } from "./a";
import sibling from './foo';
`,
...parserConfig,
options: [
{
followTsOrganizeImports: true,
},
],
}),
// test for explicit followTsOrganizeImports=false
tValid({
code: `import 'format.css';
import fs from 'node:fs';
import path from "path";
import a from 'a';
import { scopeA } from "@a/a";
import { localA } from "./a";
import sibling from './foo';
import index from './';
import { internA } from "#a";
`,
...parserConfig,
options: [
{
followTsOrganizeImports: false,
},
],
}),
// manual `groups` always take precedence over `followTsOrganizeImports`
tValid({
code: `import 'format.css';
import a from 'a';
import { scopeA } from "@a/a";
import index from './';
import fs from 'node:fs';
import path from "path";
import { localA } from "./a";
import sibling from './foo';
import { internA } from "#a";
`,
...parserConfig,
options: [
{
groups: ['external', 'internal', 'index'],
followTsOrganizeImports: true,
},
],
}),
],
invalid: [
// Option alphabetize: {order: 'asc'}
Expand Down Expand Up @@ -5192,6 +5250,30 @@ describe('TypeScript', () => {
// { message: '`node:fs/promises` import should occur before import of `express`' },
],
}),
// By default it should order same as TS LSP (issue-286)
tInvalid({
code: `import { scopeA } from "@a/a";
import fs from 'node:fs';
import path from "path";
import { localA } from "./a";
import { internA } from "#a";
`,
output: `import { internA } from "#a";
import { scopeA } from "@a/a";
import fs from 'node:fs';
import path from "path";
import { localA } from "./a";
`,
...parserConfig,
options: [
{
followTsOrganizeImports: true,
},
],
errors: [
createOrderError(['`#a` import', 'before', 'import of `@a/a`']),
],
}),
],
})
}
Expand Down