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

[New] no-duplicates: add combineTypeImports option to de-duplicate flow type imports #2229

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel
- [`no-unused-modules`]: add eslint v8 support ([#2194], thanks [@coderaiser])
- [`no-restricted-paths`]: add/restore glob pattern support ([#2219], thanks [@stropho])
- [`no-unused-modules`]: support dynamic imports ([#1660], [#2212], thanks [@maxkomarychev], [@aladdin-add], [@Hypnosphi])
- [`no-duplicates`]: add `combineTypeImports` option to de-duplicate flow type imports ([#2229], thanks [@christianvuerings])

## [2.24.2] - 2021-08-24

Expand Down Expand Up @@ -906,6 +907,7 @@ for info on changes for earlier releases.

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

[#2229]: https://github.com/import-js/eslint-plugin-import/pull/2229
[#2219]: https://github.com/import-js/eslint-plugin-import/pull/2219
[#2212]: https://github.com/import-js/eslint-plugin-import/pull/2212
[#2196]: https://github.com/import-js/eslint-plugin-import/pull/2196
Expand Down
27 changes: 23 additions & 4 deletions docs/rules/no-duplicates.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,11 @@ is different in two key ways:
Valid:
```js
import SomeDefaultClass, * as names from './mod'
// Flow `type` import from same module is fine
// By default the flow `type` import from same module is fine (see combineTypeImports)
import type SomeType from './mod'
```

...whereas here, both `./mod` imports will be reported:

Invalid:
```js
import SomeDefaultClass from './mod'

Expand All @@ -36,7 +35,8 @@ The motivation is that this is likely a result of two developers importing diffe
names from the same module at different times (and potentially largely different
locations in the file.) This rule brings both (or n-many) to attention.

### Query Strings
## Options
### considerQueryString

By default, this rule ignores query strings (i.e. paths followed by a question mark), and thus imports from `./mod?a` and `./mod?b` will be considered as duplicates. However you can use the option `considerQueryString` to handle them as different (primarily because browsers will resolve those imports differently).

Expand All @@ -61,6 +61,25 @@ import SomeDefaultClass from './mod?minify'
import * from './mod.js?minify'
```

### combineTypeImports

By default, this rule ignores flow & typescript type imports. However you can use the option `combineTypeImports` to combine type imports with regular imports.
Copy link
Member

Choose a reason for hiding this comment

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

can you explain why this is a good thing, or how it would apply to TS? In TS, it's preferred to use import type for all type imports, and import only for value imports.

Does Flow not have a separate import type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you explain why this is a good thing, or how it would apply to TS? In TS, it's preferred to use import type for all type imports, and import only for value imports.

TypeScript allows to not use import type for type imports. typescript-eslint has a rule to specify the syntax: typescript-eslint/consistent-type-imports. As you are saying it's not the preferred way, so it shouldn't be the default, but it is something users might want to set.

The main reason we would like to combine type and regular imports in our flow code is to make it easy for developers to see what the dependencies on a certain file. Some of our files have both regular and type exports so when they are imported, it's confusing to not see them combined:

Before

// @flow
import { type I18nContext } from 'app/packages/i18n/useI18n';
// Other imports
import useI18n, { I18nContextProvider } from 'app/packages/i18n/useI18n';

After

// @flow
import useI18n, { I18nContextProvider, type I18nContext } from 'app/packages/i18n/useI18n';

Does Flow not have a separate import type?

Flow supports both import type Foo and import { type Foo } syntax.

Copy link
Member

Choose a reason for hiding this comment

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

I can’t imagine why you’d prefer the combined form over the separate import vs import type statements, but at least i understand what the option would do.

Since they can’t be combined in TS, this option would only ever be useful for Flow. I’m not sure supporting only flow is worth the complexity at this time due to the current state of Flow usage in the ecosystem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ljharb understood, closing this PR for now.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like in TS 4.5+, they can be combined. I'll reopen.


Currently only `flow` is supported.

Valid:
```js
/*eslint import/no-unresolved: [2, { combineTypeImports: "flow" }]*/
import x, { type y } from './foo'
```

Invalid:
```js
/*eslint import/no-unresolved: [2, { combineTypeImports: "flow" }]*/
import { type y } from './foo' // reported since './foo' is imported twice
import { x } from './foo'
```

## When Not To Use It

If the core ESLint version is good enough (i.e. you're _not_ using Flow and you _are_ using [`import/extensions`](./extensions.md)), keep it and don't use this.
Expand Down
46 changes: 26 additions & 20 deletions src/rules/no-duplicates.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import resolve from 'eslint-module-utils/resolve';
import docsUrl from '../docsUrl';

function checkImports(imported, context) {
function checkImports(imported, context, combineTypeImports) {
for (const [module, nodes] of imported.entries()) {
if (nodes.length > 1) {
const message = `'${module}' imported multiple times.`;
const [first, ...rest] = nodes;
const sourceCode = context.getSourceCode();
const fix = getFix(first, rest, sourceCode);
const fix = getFix(first, rest, sourceCode, combineTypeImports);

context.report({
node: first.source,
Expand All @@ -25,7 +25,7 @@ function checkImports(imported, context) {
}
}

function getFix(first, rest, sourceCode) {
function getFix(first, rest, sourceCode, combineTypeImports) {
// Sorry ESLint <= 3 users, no autofix for you. Autofixing duplicate imports
// requires multiple `fixer.whatever()` calls in the `fix`: We both need to
// update the first one, and remove the rest. Support for multiple
Expand Down Expand Up @@ -107,15 +107,15 @@ function getFix(first, rest, sourceCode) {
const firstIsEmpty = !hasSpecifiers(first);

const [specifiersText] = specifiers.reduce(
([result, needsComma], specifier) => {
return [
needsComma && !specifier.isEmpty
? `${result},${specifier.text}`
: `${result}${specifier.text}`,
specifier.isEmpty ? needsComma : true,
];
},
['', !firstHasTrailingComma && !firstIsEmpty]
([result, needsComma], specifier) => [
`${result}${needsComma && !specifier.isEmpty ? ',' : ''}${
combineTypeImports === 'flow' && specifier.importNode.importKind === 'type'
? `type ${specifier.text.trim()}`
: specifier.text
}`,
specifier.isEmpty ? needsComma : true,
],
['', !firstHasTrailingComma && !firstIsEmpty],
);

const fixes = [];
Expand Down Expand Up @@ -255,18 +255,24 @@ module.exports = {
considerQueryString: {
type: 'boolean',
},
combineTypeImports: {
enum: ['flow', 'disabled'],
default: 'disabled',
},
},
additionalProperties: false,
},
],
},

create(context) {
const {
considerQueryString = false,
combineTypeImports = 'disabled',
} = context.options[0] || {};
// Prepare the resolver from options.
const considerQueryStringOption = context.options[0] &&
context.options[0]['considerQueryString'];
const defaultResolver = sourcePath => resolve(sourcePath, context) || sourcePath;
const resolver = considerQueryStringOption ? (sourcePath => {
const resolver = considerQueryString ? (sourcePath => {
const parts = sourcePath.match(/^([^?]*)\?(.*)$/);
if (!parts) {
return defaultResolver(sourcePath);
Expand All @@ -280,7 +286,7 @@ module.exports = {
const namedTypesImported = new Map();

function getImportMap(n) {
if (n.importKind === 'type') {
if (n.importKind === 'type' && combineTypeImports === 'disabled') {
return n.specifiers.length > 0 && n.specifiers[0].type === 'ImportDefaultSpecifier' ? defaultTypesImported : namedTypesImported;
}

Expand All @@ -301,10 +307,10 @@ module.exports = {
},

'Program:exit': function () {
checkImports(imported, context);
checkImports(nsImported, context);
checkImports(defaultTypesImported, context);
checkImports(namedTypesImported, context);
checkImports(imported, context, combineTypeImports);
checkImports(nsImported, context, combineTypeImports);
checkImports(defaultTypesImported, context, combineTypeImports);
checkImports(namedTypesImported, context, combineTypeImports);
},
};
},
Expand Down
47 changes: 46 additions & 1 deletion tests/src/rules/no-duplicates.js
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,51 @@ import {x,y} from './foo'
output: "import Bar, { Foo } from './foo';\nexport const value = {}",
errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'],
}),

// #2229 add option to de-duplicate flow type imports
test({
code: "import { x } from './foo'; import type { y } from './foo'",
output: "import { x ,type y} from './foo'; ",
errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'],
options: [{ 'combineTypeImports': 'flow' }],
parser: require.resolve('babel-eslint'),
}),

// #2229 add option to de-duplicate flow type imports
test({
code: "import { x } from './foo'; import type {y} from './foo'",
output: "import { x ,type y} from './foo'; ",
errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'],
options: [{ 'combineTypeImports': 'flow' }],
parser: require.resolve('babel-eslint'),
}),

// #2229 add option to de-duplicate flow type imports
test({
code: "import { type x } from './foo'; import { y } from './foo'",
output: "import { type x , y } from './foo'; ",
errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'],
options: [{ 'combineTypeImports': 'flow' }],
parser: require.resolve('babel-eslint'),
}),

// #2229 add option to de-duplicate flow type imports
test({
code: "import { type x } from './foo'; import type { y } from './foo'",
output: "import { type x ,type y} from './foo'; ",
errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'],
options: [{ 'combineTypeImports': 'flow' }],
parser: require.resolve('babel-eslint'),
}),

// #2229 add option to de-duplicate flow type imports
test({
code: "import { type x } from './foo'; import type { y } from './foo'; import type { z } from './foo'",
output: "import { type x ,type y,type z} from './foo'; ",
errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'],
options: [{ 'combineTypeImports': 'flow' }],
parser: require.resolve('babel-eslint'),
}),
],
});

Expand All @@ -429,7 +474,7 @@ context('TypeScript', function () {

ruleTester.run('no-duplicates', rule, {
valid: [
// #1667: ignore duplicate if is a typescript type import
// #1667: ignore duplicate if is a typescript type import
test({
code: "import type { x } from './foo'; import y from './foo'",
...parserConfig,
Expand Down
Loading