Skip to content

Commit

Permalink
[Fix] no-duplicates: Prefer combined type and regular imports when …
Browse files Browse the repository at this point in the history
…using `prefer-inline`
  • Loading branch information
benkrejci authored and ljharb committed Jul 19, 2023
1 parent e2cf99c commit f302f7d
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 2 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Expand Up @@ -13,6 +13,7 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange
- [`no-extraneous-dependencies`]/TypeScript: do not error when importing inline type from dev dependencies ([#1820], thanks [@andyogo])
- [`newline-after-import`]/TypeScript: do not error when re-exporting a namespaced import ([#2832], thanks [@laurens-dg])
* [`order`]: partial fix for [#2687] (thanks [@ljharb])
- [`no-duplicates`]: Detect across type and regular imports ([#2835], thanks [@benkrejci])

### Changed
- [Docs] [`no-duplicates`]: fix example schema ([#2684], thanks [@simmo])
Expand Down Expand Up @@ -1073,6 +1074,7 @@ for info on changes for earlier releases.

[`memo-parser`]: ./memo-parser/README.md

[#2835]: https://github.com/import-js/eslint-plugin-import/pull/2835
[#2832]: https://github.com/import-js/eslint-plugin-import/pull/2832
[#2756]: https://github.com/import-js/eslint-plugin-import/pull/2756
[#2754]: https://github.com/import-js/eslint-plugin-import/pull/2754
Expand Down Expand Up @@ -1647,6 +1649,7 @@ for info on changes for earlier releases.
[@BarryThePenguin]: https://github.com/BarryThePenguin
[@be5invis]: https://github.com/be5invis
[@beatrizrezener]: https://github.com/beatrizrezener
[@benkrejci]: https://github.com/benkrejci
[@benmosher]: https://github.com/benmosher
[@benmunro]: https://github.com/benmunro
[@BenoitZugmeyer]: https://github.com/BenoitZugmeyer
Expand Down
5 changes: 5 additions & 0 deletions docs/rules/no-duplicates.md
Expand Up @@ -84,12 +84,17 @@ Config:
```js
import { AValue, type AType } from './mama-mia'
import type { BType } from './mama-mia'

import { CValue } from './papa-mia'
import type { CType } from './papa-mia'
```

✅ Valid with `["error", {"prefer-inline": true}]`

```js
import { AValue, type AType, type BType } from './mama-mia'

import { CValue, type CType } from './papa-mia'
```

<!--tabs-->
Expand Down
5 changes: 3 additions & 2 deletions src/rules/no-duplicates.js
Expand Up @@ -318,10 +318,11 @@ module.exports = {
});
}
const map = moduleMaps.get(n.parent);
if (n.importKind === 'type') {
const preferInline = context.options[0] && context.options[0]['prefer-inline'];
if (!preferInline && n.importKind === 'type') {
return n.specifiers.length > 0 && n.specifiers[0].type === 'ImportDefaultSpecifier' ? map.defaultTypesImported : map.namedTypesImported;
}
if (n.specifiers.some((spec) => spec.importKind === 'type')) {
if (!preferInline && n.specifiers.some((spec) => spec.importKind === 'type')) {
return map.namedTypesImported;
}

Expand Down
19 changes: 19 additions & 0 deletions tests/src/rules/no-duplicates.js
Expand Up @@ -711,6 +711,25 @@ context('TypeScript', function () {
},
],
}),
// #2834 Detect duplicates across type and regular imports
test({
code: "import {AValue} from './foo'; import type {AType} from './foo'",

This comment has been minimized.

Copy link
@snewcomer

snewcomer Aug 6, 2023

Contributor

@benkrejci @ljharb Hi folks! There is one rule/step here that, if happens before this rule, makes this change not necessary. Once a type import is fixed to an inline type import, then the no-duplicates rule fixes it properly

#2473

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 6, 2023

Member

I’m aware these two rules may interact; I’m confused what’s not necessary?

This comment has been minimized.

Copy link
@kevinbosman

kevinbosman Aug 6, 2023

@snewcomer in our usecase, I do not want consistent-type-specifier-style, but I do want prefer-inline for when there are duplicates in type and non-type. They are not the same.

FYI the reason we do not always want inline type specifiers is because TypeScript completely removes import type { AType } from './foo' (good thing) but replaces import { type AType } from './foo' with import {} from './foo' (undesirable). This latter case also triggers @typescript-eslint/no-import-type-side-effects

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 7, 2023

Member

oof, is there no TS option to assume imports don't have side effects?

If not, then we may need to modify consistent-type-specifier-style to not recommend inline usage for imports that only pull in types.

This comment has been minimized.

Copy link
@benkrejci

benkrejci Aug 8, 2023

Author Contributor

I just want to point out in case it's not clear that unless you use the --verbatimModuleSyntax option, TypeScript will strip out any empty or type-only imports regardless of whether they are inline or not.

playground exmaple

So in projects that don't use this option, it is safe to assume that type-only imports don't have side-effects.

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 9, 2023

Member

Then we should detect this option being used, and avoid that assumption in that case.

...parserConfig,
options: [{ 'prefer-inline': true }],
output: `import {AValue,type AType} from './foo'; `,
errors: [
{
line: 1,
column: 22,
message: "'./foo' imported multiple times.",
},
{
line: 1,
column: 56,
message: "'./foo' imported multiple times.",
},
],
}),
]);

ruleTester.run('no-duplicates', rule, {
Expand Down

0 comments on commit f302f7d

Please sign in to comment.