fix: backports upstream changes (#148)

* fix: backports import-js#3033

* ci: cancel-in-progress when pr is updated

backports import-js#2997

* fix: backports import-js#2952

* docs: improve `order` rule docs

Backports:
- import-js#2640
- import-js#3036

.changeset/chilled-pandas-tickle.md

@@ -0,0 +1,5 @@
+---
+"eslint-plugin-import-x": patch
+---
+
+Fix `newline-after-import`'s `considerComments` options when linting `require`, backports https://github.com/import-js/eslint-plugin-import/pull/2952

.changeset/weak-shirts-smell.md

@@ -0,0 +1,5 @@
+---
+"eslint-plugin-import-x": patch
+---
+
+Fix `no-duplicates` for TypeScript, backports https://github.com/import-js/eslint-plugin-import/pull/3033

.github/workflows/ci.yml

@@ -4,6 +4,10 @@ on:
   - push
   - pull_request
 
+concurrency:
+  group: ${{ github.workflow }}-${{ github.ref }}
+  cancel-in-progress: ${{ github.event_name == 'pull_request' }}
+
 jobs:
   ci:
     name: Lint and Test with Node.js ${{ matrix.node }} and ESLint ${{ matrix.eslint }} on ${{ matrix.os }}

docs/rules/order.md

@@ -77,6 +77,25 @@ import foo from './foo'
 var path = require('path')
 ```
 
+## Limitations of `--fix`
+
+Unbound imports are assumed to have side effects, and will never be moved/reordered. This can cause other imports to get "stuck" around them, and the fix to fail.
+
+```javascript
+import b from 'b'
+import 'format.css' // This will prevent --fix from working.
+import a from 'a'
+```
+
+As a workaround, move unbound imports to be entirely above or below bound ones.
+
+```javascript
+import 'format1.css' // OK
+import b from 'b'
+import a from 'a'
+import 'format2.css' // OK
+```
+
 ## Options
 
 This rule supports the following options:
@@ -180,7 +199,8 @@ Example:
 ### `pathGroupsExcludedImportTypes: [array]`
 
 This defines import types that are not handled by configured pathGroups.
-This is mostly needed when you want to handle path groups that look like external imports.
+
+If you have added path groups with patterns that look like `"builtin"` or `"external"` imports, you have to remove this group (`"builtin"` and/or `"external"`) from the default exclusion list (e.g., `["builtin", "external", "object"]`, etc) to sort these path groups correctly.
 
 Example:
 
@@ -202,29 +222,7 @@ Example:
 }
 ```
 
-You can also use `patterns`(e.g., `react`, `react-router-dom`, etc).
-
-Example:
-
-```json
-{
-  "import-x/order": [
-    "error",
-    {
-      "pathGroups": [
-        {
-          "pattern": "react",
-          "group": "builtin",
-          "position": "before"
-        }
-      ],
-      "pathGroupsExcludedImportTypes": ["react"]
-    }
-  ]
-}
-```
-
-The default value is `["builtin", "external", "object"]`.
+[Import Type](https://github.com/un-ts/eslint-plugin-import-x/blob/ea7c13eb9b18357432e484b25dfa4451eca69c5b/src/utils/import-type.ts#L145) is resolved as a fixed string in predefined set, it can't be a `patterns` (e.g., `react`, `react-router-dom`, etc).
 
 ### `newlines-between: [ignore|always|always-and-inside-groups|never]`
 

src/rules/newline-after-import.ts

@@ -177,6 +177,7 @@ export = createRule<[Options?], MessageId>({
     function commentAfterImport(
       node: TSESTree.Node,
       nextComment: TSESTree.Comment,
+      type: 'import' | 'require',
     ) {
       const lineDifference = getLineDifference(node, nextComment)
       const EXPECTED_LINE_DIFFERENCE = options.count + 1
@@ -197,7 +198,7 @@ export = createRule<[Options?], MessageId>({
           data: {
             count: options.count,
             lineSuffix: options.count > 1 ? 's' : '',
-            type: 'import',
+            type,
           },
           fix:
             options.exactCount && EXPECTED_LINE_DIFFERENCE < lineDifference
@@ -253,7 +254,7 @@ export = createRule<[Options?], MessageId>({
       }
 
       if (nextComment) {
-        commentAfterImport(node, nextComment)
+        commentAfterImport(node, nextComment, 'import')
       } else if (
         nextNode &&
         nextNode.type !== 'ImportDeclaration' &&
@@ -301,7 +302,33 @@ export = createRule<[Options?], MessageId>({
             (!nextRequireCall ||
               !containsNodeOrEqual(nextStatement, nextRequireCall))
           ) {
-            checkForNewLine(statementWithRequireCall, nextStatement, 'require')
+            let nextComment
+            if (
+              'comments' in statementWithRequireCall.parent &&
+              statementWithRequireCall.parent.comments !== undefined &&
+              options.considerComments
+            ) {
+              const endLine = node.loc.end.line
+              nextComment = statementWithRequireCall.parent.comments.find(
+                o =>
+                  o.loc.start.line >= endLine &&
+                  o.loc.start.line <= endLine + options.count + 1,
+              )
+            }
+
+            if (nextComment && nextComment !== undefined) {
+              commentAfterImport(
+                statementWithRequireCall,
+                nextComment,
+                'require',
+              )
+            } else {
+              checkForNewLine(
+                statementWithRequireCall,
+                nextStatement,
+                'require',
+              )
+            }
           }
         }
       },

src/rules/no-duplicates.ts

@@ -200,6 +200,32 @@ function getFix(
 
     const fixes = []
 
+    if (shouldAddSpecifiers && preferInline && first.importKind === 'type') {
+      // `import type {a} from './foo'` → `import {type a} from './foo'`
+      const typeIdentifierToken = tokens.find(
+        token => token.type === 'Identifier' && token.value === 'type',
+      )
+      if (typeIdentifierToken) {
+        fixes.push(
+          fixer.removeRange([
+            typeIdentifierToken.range[0],
+            typeIdentifierToken.range[1] + 1,
+          ]),
+        )
+      }
+
+      for (const identifier of tokens.filter(token =>
+        firstExistingIdentifiers.has(token.value),
+      )) {
+        fixes.push(
+          fixer.replaceTextRange(
+            [identifier.range[0], identifier.range[1]],
+            `type ${identifier.value}`,
+          ),
+        )
+      }
+    }
+
     if (openBrace == null && shouldAddSpecifiers && shouldAddDefault()) {
       // `import './foo'` → `import def, {...} from './foo'`
       fixes.push(

test/rules/newline-after-import.spec.ts

@@ -260,7 +260,7 @@ ruleTester.run('newline-after-import', rule, {
       options: [{ count: 4, exactCount: true }],
     },
     {
-      code: `var foo = require('foo-module');\n\n\n\n// Some random comment\nvar foo = 'bar';`,
+      code: `var foo = require('foo-module');\n\n\n\n\n// Some random comment\nvar foo = 'bar';`,
       languageOptions: {
         parserOptions: { ecmaVersion: 2015, sourceType: 'module' },
       },
@@ -479,6 +479,21 @@ ruleTester.run('newline-after-import', rule, {
         parserOptions: { ecmaVersion: 2015, sourceType: 'module' },
       },
     },
+    {
+      code: `var foo = require('foo-module');\n\n\n// Some random comment\nvar foo = 'bar';`,
+      options: [{ count: 2, considerComments: true }],
+    },
+    {
+      code: `var foo = require('foo-module');\n\n\n/**\n * Test comment\n */\nvar foo = 'bar';`,
+      options: [{ count: 2, considerComments: true }],
+    },
+    {
+      code: `const foo = require('foo');\n\n\n// some random comment\nconst bar = function() {};`,
+      options: [{ count: 2, exactCount: true, considerComments: true }],
+      languageOptions: {
+        parserOptions: { ecmaVersion: 2015 },
+      },
+    },
   ],
 
   invalid: [
@@ -1054,17 +1069,39 @@ ruleTester.run('newline-after-import', rule, {
       },
     },
     {
-      code: `const foo = require('foo');\n\n\n// some random comment\nconst bar = function() {};`,
-      output: null,
-      options: [{ count: 2, exactCount: true, considerComments: true }],
+      code: `var foo = require('foo-module');\nvar foo = require('foo-module');\n\n// Some random comment\nvar foo = 'bar';`,
+      output: `var foo = require('foo-module');\nvar foo = require('foo-module');\n\n\n// Some random comment\nvar foo = 'bar';`,
+      errors: [
+        {
+          line: 2,
+          column: 1,
+          messageId: `newline`,
+        },
+      ],
+      languageOptions: {
+        parserOptions: {
+          ecmaVersion: 2015,
+          sourceType: 'module',
+        },
+      },
+      options: [{ considerComments: true, count: 2 }],
+    },
+    {
+      code: `var foo = require('foo-module');\n\n/**\n * Test comment\n */\nvar foo = 'bar';`,
+      output: `var foo = require('foo-module');\n\n\n/**\n * Test comment\n */\nvar foo = 'bar';`,
       errors: [
         {
           line: 1,
           column: 1,
-          ...getRequireError(2),
+          messageId: `newline`,
         },
       ],
-      languageOptions: { parserOptions: { ecmaVersion: 2015 } },
+      languageOptions: {
+        parserOptions: {
+          ecmaVersion: 2015,
+        },
+      },
+      options: [{ considerComments: true, count: 2 }],
     },
   ],
 })

test/rules/no-duplicates.spec.ts

@@ -757,6 +757,24 @@ describe('TypeScript', () => {
               },
             ],
           }),
+          test({
+            code: "import type {x} from 'foo'; import {type y} from 'foo'",
+            ...parserConfig,
+            options: [{ 'prefer-inline': true }],
+            output: `import {type x,type y} from 'foo'; `,
+            errors: [
+              {
+                line: 1,
+                column: 22,
+                message: "'foo' imported multiple times.",
+              },
+              {
+                line: 1,
+                column: 50,
+                message: "'foo' imported multiple times.",
+              },
+            ],
+          }),
           test({
             code: "import {type x} from 'foo'; import type {y} from 'foo'",
             ...parserConfig,