From a33d27bc80ef8207a0c0763ac62f0668b1628b91 Mon Sep 17 00:00:00 2001 From: cmal <{ID}+{username}@users.noreply.github.com> Date: Mon, 29 Jun 2020 11:32:30 +0800 Subject: [PATCH] Fix: ignore unmergable imports when checking no-duplicate-imports * Fix: typo in docs/rules/no-duplicate-imports.md --- docs/rules/no-duplicate-imports.md | 25 +++++- lib/rules/no-duplicate-imports.js | 107 ++++++++++++++++++++++-- tests/lib/rules/no-duplicate-imports.js | 13 +++ 3 files changed, 135 insertions(+), 10 deletions(-) diff --git a/docs/rules/no-duplicate-imports.md b/docs/rules/no-duplicate-imports.md index 683f31584a1..a864a958104 100644 --- a/docs/rules/no-duplicate-imports.md +++ b/docs/rules/no-duplicate-imports.md @@ -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: @@ -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`. diff --git a/lib/rules/no-duplicate-imports.js b/lib/rules/no-duplicate-imports.js index 7218dc64add..0cda3e3e560 100644 --- a/lib/rules/no-duplicate-imports.js +++ b/lib/rules/no-duplicate-imports.js @@ -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 + ); +} + + +/** + * 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") + ) + ); +} + +/** + * 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, @@ -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. */ @@ -62,13 +151,13 @@ 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); } }; } @@ -76,8 +165,8 @@ function handleImports(context, includeExports, importsInFile, exportsInFile) { /** * 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. */ @@ -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); } }; } diff --git a/tests/lib/rules/no-duplicate-imports.js b/tests/lib/rules/no-duplicate-imports.js index 42d35c85751..ed9be12dc34 100644 --- a/tests/lib/rules/no-duplicate-imports.js +++ b/tests/lib/rules/no-duplicate-imports.js @@ -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 }] @@ -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" }]