Skip to content
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

fix: backports upstream changes #148

Merged
merged 4 commits into from
Sep 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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/chilled-pandas-tickle.md
Original file line number Diff line number Diff line change
@@ -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
5 changes: 5 additions & 0 deletions .changeset/weak-shirts-smell.md
Original file line number Diff line number Diff line change
@@ -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
4 changes: 4 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
Expand Down
46 changes: 22 additions & 24 deletions docs/rules/order.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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:

Expand All @@ -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]`

Expand Down
33 changes: 30 additions & 3 deletions src/rules/newline-after-import.ts
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@
function commentAfterImport(
node: TSESTree.Node,
nextComment: TSESTree.Comment,
type: 'import' | 'require',
) {
const lineDifference = getLineDifference(node, nextComment)
const EXPECTED_LINE_DIFFERENCE = options.count + 1
Expand All @@ -197,7 +198,7 @@
data: {
count: options.count,
lineSuffix: options.count > 1 ? 's' : '',
type: 'import',
type,
},
fix:
options.exactCount && EXPECTED_LINE_DIFFERENCE < lineDifference
Expand Down Expand Up @@ -253,7 +254,7 @@
}

if (nextComment) {
commentAfterImport(node, nextComment)
commentAfterImport(node, nextComment, 'import')
} else if (
nextNode &&
nextNode.type !== 'ImportDeclaration' &&
Expand Down Expand Up @@ -301,7 +302,33 @@
(!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) {
Dismissed Show dismissed Hide dismissed
commentAfterImport(
statementWithRequireCall,
nextComment,
'require',
)
} else {
checkForNewLine(
statementWithRequireCall,
nextStatement,
'require',
)
}
}
}
},
Expand Down
26 changes: 26 additions & 0 deletions src/rules/no-duplicates.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
49 changes: 43 additions & 6 deletions test/rules/newline-after-import.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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' },
},
Expand Down Expand Up @@ -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: [
Expand Down Expand Up @@ -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 }],
},
],
})
18 changes: 18 additions & 0 deletions test/rules/no-duplicates.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading