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.

Sign up for GitHub

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

Jump to bottom

Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) #13180

Closed
wants to merge 1 commit into from
Closed

Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) #13180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 24 additions & 1 deletion docs/rules/no-duplicate-imports.md
View file
Open in desktop
Expand Up @@ -12,7 +12,9 @@ import { find } from 'module';

## Rule Details

This rule requires that all imports from a single module exists in a single `import` statement.
An import that can be merged with another is a duplicate of that other.

This rule requires that all imports from a single module that can be merged exists in a single `import` statement.

Example of **incorrect** code for this rule:

Expand All @@ -33,6 +35,27 @@ import { merge, find } from 'module';
import something from 'another-module';
```

The following examples explains **can be merged** in detail:

```js
// mergable, as parsers1 & parsers2 hold the same reference, and so their identifiers can be replaced.
import * as parsers1 from 'parsers';
import * as parsers2 from 'parsers';

// mergable, as they can be combined without changing behaviour & remaining syntactically valid.
import defaultValue from 'parsers';
import { anotherValue } from 'parsers';

// mergable as they will both trigger any side effects the module has.
import * as parsers2 from 'parsers';
import 'parsers';

// not mergable, as they would require new nodes to be created.
import { anotherValue } from 'parsers';
import * as parsers1 from 'parsers';
```


## Options

This rule takes one optional argument, an object with a single key, `includeExports` which is a `boolean`. It defaults to `false`.
Expand Down
107 changes: 98 additions & 9 deletions lib/rules/no-duplicate-imports.js
View file
Open in desktop
Expand Up @@ -21,18 +21,107 @@ function getValue(node) {
return "";
}

/**
* Checks if the import's type is the given type import.
* @param {ASTNode} node The import to be checked.
* @param {type} type the type to be checked.
*
* @returns {boolean} Whether or not the import's type is type.
*/
function checkImportType(node, type) {
return (
node.specifiers &&
node.specifiers[0] &&
node.specifiers[0].type === type
);
}
cmal marked this conversation as resolved.
Show resolved Hide resolved


/**
* Checks whether a node has the given value.
* @param {string} value the value to be checked.
*
* @returns {Function} the predicate function, return true if the parameter ASTNode has the given value.
*/
function hasValue(value) {
return function(node) {
return getValue(node) === value;
};
}

/**
* Checks if two nodes can be merged.
* @param {ASTNode} node1 one of the two nodes to be checked.
* @param {ASTNode} node2 the other node to be checked.
*
* @returns {boolean} true where only one ImportNamespaceSpecifier and one ImportSpecifier, otherwise, false.
*/
function isTwoNodesCanMerge(node1, node2) {
return (
(
checkImportType(node1, "ImportNamespaceSpecifier") &&
checkImportType(node2, "ImportSpecifier")
) ||
(
checkImportType(node1, "ImportSpecifier") &&
checkImportType(node2, "ImportNamespaceSpecifier")
)
);
}
Comment on lines +59 to +70
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 this function is actually doing the opposite of its name: checks if two nodes cannot be merged?


/**
* Checks if one node cannot be merged into the imports, which means a duplication.
* @param {ASTNode} node the node to be checked.
* @param {ASTNode[]} importsInFile the imports to be checked.
*
* @returns {boolean} true if the node can be merged.
*/
function canBeMerged(node, importsInFile) {
const sameSourceImports = importsInFile.filter(hasValue(getValue(node)));

return !(
!sameSourceImports.length ||
(
sameSourceImports.length === 1 &&
isTwoNodesCanMerge(node, sameSourceImports[0])
)
);
}

/**
* Checks if the name of the import or export exists in the given array, and reports if so.
* @param {RuleContext} context The ESLint rule context object.
* @param {ASTNode} node A node to get.
* @param {string} value The name of the imported or exported module.
* @param {ASTNode[]} importsInFile The array containing other imports or exports in the file.
* @param {string} messageId A messageId to be reported after the name of the module
*
* @returns {void} No return value
*/
function checkAndReportImport(context, node, value, importsInFile, messageId) {
if (canBeMerged(node, importsInFile)) {
context.report({
node,
messageId,
data: {
module: value
}
});
}
}

/**
* Checks if the name of the import or export exists in the given array, and reports if so.
* @param {RuleContext} context The ESLint rule context object.
* @param {ASTNode} node A node to get.
* @param {string} value The name of the imported or exported module.
* @param {string[]} array The array containing other imports or exports in the file.
* @param {ASTNode[]} array The array containing other imports or exports in the file.
* @param {string} messageId A messageId to be reported after the name of the module
*
* @returns {void} No return value
*/
function checkAndReport(context, node, value, array, messageId) {
if (array.indexOf(value) !== -1) {
if (array.map(getValue).indexOf(value) !== -1) {
context.report({
node,
messageId,
Expand All @@ -52,8 +141,8 @@ function checkAndReport(context, node, value, array, messageId) {
* Returns a function handling the imports of a given file
* @param {RuleContext} context The ESLint rule context object.
* @param {boolean} includeExports Whether or not to check for exports in addition to imports.
* @param {string[]} importsInFile The array containing other imports in the file.
* @param {string[]} exportsInFile The array containing other exports in the file.
* @param {ASTNode[]} importsInFile The array containing other imports in the file.
* @param {ASTNode[]} exportsInFile The array containing other exports in the file.
*
* @returns {nodeCallback} A function passed to ESLint to handle the statement.
*/
Expand All @@ -62,22 +151,22 @@ function handleImports(context, includeExports, importsInFile, exportsInFile) {
const value = getValue(node);

if (value) {
checkAndReport(context, node, value, importsInFile, "import");
checkAndReportImport(context, node, value, importsInFile, "import");

if (includeExports) {
checkAndReport(context, node, value, exportsInFile, "importAs");
}

importsInFile.push(value);
importsInFile.push(node);
}
};
}

/**
* Returns a function handling the exports of a given file
* @param {RuleContext} context The ESLint rule context object.
* @param {string[]} importsInFile The array containing other imports in the file.
* @param {string[]} exportsInFile The array containing other exports in the file.
* @param {ASTNode[]} importsInFile The array containing other imports in the file.
* @param {ASTNode[]} exportsInFile The array containing other exports in the file.
*
* @returns {nodeCallback} A function passed to ESLint to handle the statement.
*/
Expand All @@ -89,7 +178,7 @@ function handleExports(context, importsInFile, exportsInFile) {
checkAndReport(context, node, value, exportsInFile, "export");
checkAndReport(context, node, value, importsInFile, "exportAs");

exportsInFile.push(value);
exportsInFile.push(node);
}
};
}
Expand Down
13 changes: 13 additions & 0 deletions tests/lib/rules/no-duplicate-imports.js
View file
Open in desktop
Expand Up @@ -26,6 +26,7 @@ ruleTester.run("no-duplicate-imports", rule, {
"import * as Foobar from \"async\";",
"import \"foo\"",
"import os from \"os\";\nexport { something } from \"os\";",
"import * as Lodash from \"lodash-es\";import { merge } from \"lodash-es\";",
{
code: "import os from \"os\";\nexport { hello } from \"hello\";",
options: [{ includeExports: true }]
Expand All @@ -48,6 +49,18 @@ ruleTester.run("no-duplicate-imports", rule, {
}
],
invalid: [
{
code: "import * as Foo from \"os\";\nimport * as Bar from \"os\"",
errors: [{ messageId: "import", data: { module: "os" }, type: "ImportDeclaration" }]
},
{
code: "import os from \"os\";\nimport { arch } from \"os\"",
errors: [{ messageId: "import", data: { module: "os" }, type: "ImportDeclaration" }]
},
{
code: "import * as Foobar from \"os\";\nimport \"os\"",
errors: [{ messageId: "import", data: { module: "os" }, type: "ImportDeclaration" }]
},
{
code: "import \"fs\";\nimport \"fs\"",
errors: [{ messageId: "import", data: { module: "fs" }, type: "ImportDeclaration" }]
Expand Down