diff --git a/docs/rules/no-duplicate-imports.md b/docs/rules/no-duplicate-imports.md index 683f31584a1..437d2c3daa1 100644 --- a/docs/rules/no-duplicate-imports.md +++ b/docs/rules/no-duplicate-imports.md @@ -12,7 +12,7 @@ import { find } from 'module'; ## Rule Details -This rule requires that all imports from a single module exists in a single `import` statement. +This rule requires that all imports from a single module that can be merged exist in a single `import` statement. Example of **incorrect** code for this rule: @@ -33,6 +33,16 @@ import { merge, find } from 'module'; import something from 'another-module'; ``` +Example of **correct** code for this rule: + +```js +/*eslint no-duplicate-imports: "error"*/ + +// not mergeable +import { merge } from 'module'; +import * as something from 'module'; +``` + ## Options This rule takes one optional argument, an object with a single key, `includeExports` which is a `boolean`. It defaults to `false`. @@ -58,3 +68,17 @@ import { merge, find } from 'module'; export { find }; ``` + +Example of **correct** code for this rule with the `{ "includeExports": true }` option: + +```js +/*eslint no-duplicate-imports: ["error", { "includeExports": true }]*/ + +import { merge, find } from 'module'; + +// cannot be merged with the above import +export * as something from 'module'; + +// cannot be written differently +export * from 'module'; +``` diff --git a/lib/rules/no-duplicate-imports.js b/lib/rules/no-duplicate-imports.js index 7218dc64add..cc3da1d5a68 100644 --- a/lib/rules/no-duplicate-imports.js +++ b/lib/rules/no-duplicate-imports.js @@ -4,92 +4,225 @@ */ "use strict"; +//------------------------------------------------------------------------------ +// Helpers +//------------------------------------------------------------------------------ + +const NAMED_TYPES = ["ImportSpecifier", "ExportSpecifier"]; +const NAMESPACE_TYPES = [ + "ImportNamespaceSpecifier", + "ExportNamespaceSpecifier" +]; + //------------------------------------------------------------------------------ // Rule Definition //------------------------------------------------------------------------------ /** - * Returns the name of the module imported or re-exported. + * Check if an import/export type belongs to (ImportSpecifier|ExportSpecifier) or (ImportNamespaceSpecifier|ExportNamespaceSpecifier). + * @param {string} importExportType An import/export type to check. + * @param {string} type Can be "named" or "namespace" + * @returns {boolean} True if import/export type belongs to (ImportSpecifier|ExportSpecifier) or (ImportNamespaceSpecifier|ExportNamespaceSpecifier) and false if it doesn't. + */ +function isImportExportSpecifier(importExportType, type) { + const arrayToCheck = type === "named" ? NAMED_TYPES : NAMESPACE_TYPES; + + return arrayToCheck.includes(importExportType); +} + +/** + * Return the type of (import|export). * @param {ASTNode} node A node to get. - * @returns {string} the name of the module, or empty string if no name. + * @returns {string} The type of the (import|export). */ -function getValue(node) { - if (node && node.source && node.source.value) { - return node.source.value.trim(); +function getImportExportType(node) { + if (node.specifiers && node.specifiers.length > 0) { + const nodeSpecifiers = node.specifiers; + const index = nodeSpecifiers.findIndex( + ({ type }) => + isImportExportSpecifier(type, "named") || + isImportExportSpecifier(type, "namespace") + ); + const i = index > -1 ? index : 0; + + return nodeSpecifiers[i].type; } + if (node.type === "ExportAllDeclaration") { + if (node.exported) { + return "ExportNamespaceSpecifier"; + } + return "ExportAll"; + } + return "SideEffectImport"; +} - return ""; +/** + * Returns a boolean indicates if two (import|export) can be merged + * @param {ASTNode} node1 A node to check. + * @param {ASTNode} node2 A node to check. + * @returns {boolean} True if two (import|export) can be merged, false if they can't. + */ +function isImportExportCanBeMerged(node1, node2) { + const importExportType1 = getImportExportType(node1); + const importExportType2 = getImportExportType(node2); + + if ( + (importExportType1 === "ExportAll" && + importExportType2 !== "ExportAll" && + importExportType2 !== "SideEffectImport") || + (importExportType1 !== "ExportAll" && + importExportType1 !== "SideEffectImport" && + importExportType2 === "ExportAll") + ) { + return false; + } + if ( + (isImportExportSpecifier(importExportType1, "namespace") && + isImportExportSpecifier(importExportType2, "named")) || + (isImportExportSpecifier(importExportType2, "namespace") && + isImportExportSpecifier(importExportType1, "named")) + ) { + return false; + } + return true; } /** - * 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 {string} messageId A messageId to be reported after the name of the module - * - * @returns {void} No return value + * Returns a boolean if we should report (import|export). + * @param {ASTNode} node A node to be reported or not. + * @param {[ASTNode]} previousNodes An array contains previous nodes of the module imported or exported. + * @returns {boolean} True if the (import|export) should be reported. */ -function checkAndReport(context, node, value, array, messageId) { - if (array.indexOf(value) !== -1) { - context.report({ - node, - messageId, - data: { - module: value - } - }); +function shouldReportImportExport(node, previousNodes) { + let i = 0; + + while (i < previousNodes.length) { + if (isImportExportCanBeMerged(node, previousNodes[i])) { + return true; + } + i++; } + return false; } /** - * @callback nodeCallback - * @param {ASTNode} node A node to handle. + * Returns array contains only nodes with declarations types equal to type. + * @param {[{node: ASTNode, declarationType: string}]} nodes An array contains objects, each object contains a node and a declaration type. + * @param {string} type Declaration type. + * @returns {[ASTNode]} An array contains only nodes with declarations types equal to type. + */ +function getNodesByDeclarationType(nodes, type) { + return nodes + .filter(({ declarationType }) => declarationType === type) + .map(({ node }) => node); +} + +/** + * Returns the name of the module imported or re-exported. + * @param {ASTNode} node A node to get. + * @returns {string} The name of the module, or empty string if no name. */ +function getModule(node) { + if (node && node.source && node.source.value) { + return node.source.value.trim(); + } + return ""; +} /** - * Returns a function handling the imports of a given file + * Checks if the (import|export) can be merged with at least one import or one export, and reports if so. * @param {RuleContext} context The ESLint rule context object. + * @param {ASTNode} node A node to get. + * @param {Map} modules A Map object contains as a key a module name and as value an array contains objects, each object contains a node and a declaration type. + * @param {string} declarationType A declaration type can be an import or export. * @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. - * - * @returns {nodeCallback} A function passed to ESLint to handle the statement. + * @returns {void} No return value. */ -function handleImports(context, includeExports, importsInFile, exportsInFile) { - return function(node) { - const value = getValue(node); +function checkAndReport( + context, + node, + modules, + declarationType, + includeExports +) { + const module = getModule(node); - if (value) { - checkAndReport(context, node, value, importsInFile, "import"); + if (modules.has(module)) { + const previousNodes = modules.get(module); + const messagesIds = []; + const importNodes = getNodesByDeclarationType(previousNodes, "import"); + let exportNodes; + if (includeExports) { + exportNodes = getNodesByDeclarationType(previousNodes, "export"); + } + if (declarationType === "import") { + if (shouldReportImportExport(node, importNodes)) { + messagesIds.push("import"); + } if (includeExports) { - checkAndReport(context, node, value, exportsInFile, "importAs"); + if (shouldReportImportExport(node, exportNodes)) { + messagesIds.push("importAs"); + } + } + } else if (declarationType === "export") { + if (shouldReportImportExport(node, exportNodes)) { + messagesIds.push("export"); + } + if (shouldReportImportExport(node, importNodes)) { + messagesIds.push("exportAs"); } - - importsInFile.push(value); } - }; + messagesIds.forEach(messageId => + context.report({ + node, + messageId, + data: { + module + } + })); + } } /** - * Returns a function handling the exports of a given file + * @callback nodeCallback + * @param {ASTNode} node A node to handle. + */ + +/** + * Returns a function handling the (imports|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 {Map} modules A Map object contains as a key a module name and as value an array contains objects, each object contains a node and a declaration type. + * @param {string} declarationType A declaration type can be an import or export. + * @param {boolean} includeExports Whether or not to check for exports in addition to imports. * @returns {nodeCallback} A function passed to ESLint to handle the statement. */ -function handleExports(context, importsInFile, exportsInFile) { +function handleImportsExports( + context, + modules, + declarationType, + includeExports +) { return function(node) { - const value = getValue(node); + const module = getModule(node); + + if (module) { + checkAndReport( + context, + node, + modules, + declarationType, + includeExports + ); + const currentNode = { node, declarationType }; + let nodes = [currentNode]; - if (value) { - checkAndReport(context, node, value, exportsInFile, "export"); - checkAndReport(context, node, value, importsInFile, "exportAs"); + if (modules.has(module)) { + const previousNodes = modules.get(module); - exportsInFile.push(value); + nodes = [...previousNodes, currentNode]; + } + modules.set(module, nodes); } }; } @@ -105,16 +238,19 @@ module.exports = { url: "https://eslint.org/docs/rules/no-duplicate-imports" }, - schema: [{ - type: "object", - properties: { - includeExports: { - type: "boolean", - default: false - } - }, - additionalProperties: false - }], + schema: [ + { + type: "object", + properties: { + includeExports: { + type: "boolean", + default: false + } + }, + additionalProperties: false + } + ], + messages: { import: "'{{module}}' import is duplicated.", importAs: "'{{module}}' import is duplicated as export.", @@ -125,18 +261,30 @@ module.exports = { create(context) { const includeExports = (context.options[0] || {}).includeExports, - importsInFile = [], - exportsInFile = []; - + modules = new Map(); const handlers = { - ImportDeclaration: handleImports(context, includeExports, importsInFile, exportsInFile) + ImportDeclaration: handleImportsExports( + context, + modules, + "import", + includeExports + ) }; if (includeExports) { - handlers.ExportNamedDeclaration = handleExports(context, importsInFile, exportsInFile); - handlers.ExportAllDeclaration = handleExports(context, importsInFile, exportsInFile); + handlers.ExportNamedDeclaration = handleImportsExports( + context, + modules, + "export", + includeExports + ); + handlers.ExportAllDeclaration = handleImportsExports( + context, + modules, + "export", + includeExports + ); } - return handlers; } }; diff --git a/tests/lib/rules/no-duplicate-imports.js b/tests/lib/rules/no-duplicate-imports.js index 42d35c85751..e200bbc704d 100644 --- a/tests/lib/rules/no-duplicate-imports.js +++ b/tests/lib/rules/no-duplicate-imports.js @@ -16,7 +16,7 @@ const rule = require("../../../lib/rules/no-duplicate-imports"), // Tests //------------------------------------------------------------------------------ -const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 6, sourceType: "module" } }); +const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 12, sourceType: "module" } }); ruleTester.run("no-duplicate-imports", rule, { valid: [ @@ -26,6 +26,9 @@ ruleTester.run("no-duplicate-imports", rule, { "import * as Foobar from \"async\";", "import \"foo\"", "import os from \"os\";\nexport { something } from \"os\";", + "import * as bar from \"os\";\nimport { baz } from \"os\";", + "import foo, * as bar from \"os\";\nimport { baz } from \"os\";", + "import foo, { bar } from \"os\";\nimport * as baz from \"os\";", { code: "import os from \"os\";\nexport { hello } from \"hello\";", options: [{ includeExports: true }] @@ -45,6 +48,26 @@ ruleTester.run("no-duplicate-imports", rule, { { code: "import { merge } from \"lodash-es\";\nexport { merge as lodashMerge }", options: [{ includeExports: true }] + }, + { + code: "export { something } from \"os\";\nexport * as os from \"os\";", + options: [{ includeExports: true }] + }, + { + code: "import { something } from \"os\";\nexport * as os from \"os\";", + options: [{ includeExports: true }] + }, + { + code: "import * as os from \"os\";\nexport { something } from \"os\";", + options: [{ includeExports: true }] + }, + { + code: "import os from \"os\";\nexport * from \"os\";", + options: [{ includeExports: true }] + }, + { + code: "export { something } from \"os\";\nexport * from \"os\";", + options: [{ includeExports: true }] } ], invalid: [ @@ -53,11 +76,22 @@ ruleTester.run("no-duplicate-imports", rule, { errors: [{ messageId: "import", data: { module: "fs" }, type: "ImportDeclaration" }] }, { - code: "import { merge } from \"lodash-es\";import { find } from \"lodash-es\";", + code: "import { merge } from \"lodash-es\";\nimport { find } from \"lodash-es\";", errors: [{ messageId: "import", data: { module: "lodash-es" }, type: "ImportDeclaration" }] }, { - code: "import { merge } from \"lodash-es\";import _ from \"lodash-es\";", + code: "import { merge } from \"lodash-es\";\nimport _ from \"lodash-es\";", + errors: [{ messageId: "import", data: { module: "lodash-es" }, type: "ImportDeclaration" }] + }, + { + code: "import os from \"os\";\nimport { something } from \"os\";\nimport * as foobar from \"os\";", + errors: [ + { messageId: "import", data: { module: "os" }, type: "ImportDeclaration" }, + { messageId: "import", data: { module: "os" }, type: "ImportDeclaration" } + ] + }, + { + code: "import * as modns from \"lodash-es\";\nimport { merge } from \"lodash-es\";\nimport { baz } from \"lodash-es\";", errors: [{ messageId: "import", data: { module: "lodash-es" }, type: "ImportDeclaration" }] }, { @@ -66,7 +100,7 @@ ruleTester.run("no-duplicate-imports", rule, { errors: [{ messageId: "export", data: { module: "os" }, type: "ExportNamedDeclaration" }] }, { - code: "import os from \"os\"; export { os as foobar } from \"os\";\nexport { something } from \"os\";", + code: "import os from \"os\";\nexport { os as foobar } from \"os\";\nexport { something } from \"os\";", options: [{ includeExports: true }], errors: [ { messageId: "exportAs", data: { module: "os" }, type: "ExportNamedDeclaration" }, @@ -80,7 +114,27 @@ ruleTester.run("no-duplicate-imports", rule, { errors: [{ messageId: "exportAs", data: { module: "os" }, type: "ExportNamedDeclaration" }] }, { - code: "import os from \"os\";\nexport * from \"os\";", + code: "import os from \"os\";\nexport * as os from \"os\";", + options: [{ includeExports: true }], + errors: [{ messageId: "exportAs", data: { module: "os" }, type: "ExportAllDeclaration" }] + }, + { + code: "export * as os from \"os\";\nimport os from \"os\";", + options: [{ includeExports: true }], + errors: [{ messageId: "importAs", data: { module: "os" }, type: "ImportDeclaration" }] + }, + { + code: "import * as modns from \"mod\";\nexport * as modns from \"mod\";", + options: [{ includeExports: true }], + errors: [{ messageId: "exportAs", data: { module: "mod" }, type: "ExportAllDeclaration" }] + }, + { + code: "export * from \"os\";\nexport * from \"os\";", + options: [{ includeExports: true }], + errors: [{ messageId: "export", data: { module: "os" }, type: "ExportAllDeclaration" }] + }, + { + code: "import \"os\";\nexport * from \"os\";", options: [{ includeExports: true }], errors: [{ messageId: "exportAs", data: { module: "os" }, type: "ExportAllDeclaration" }] }