From e20fab5a33a279fddf839a3e773f1d80ecc7ec27 Mon Sep 17 00:00:00 2001 From: soufianeboutahlil Date: Sun, 21 Mar 2021 19:22:37 +0100 Subject: [PATCH] Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760) --- docs/rules/no-duplicate-imports.md | 23 ++- lib/rules/no-duplicate-imports.js | 235 ++++++++++++++++++++---- tests/lib/rules/no-duplicate-imports.js | 211 +++++++++++++++++---- 3 files changed, 396 insertions(+), 73 deletions(-) diff --git a/docs/rules/no-duplicate-imports.md b/docs/rules/no-duplicate-imports.md index 683f31584a1..ea198721a0d 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,14 @@ import { merge, find } from 'module'; import something from 'another-module'; ``` +Example of **correct** code for this rule: + +```js +// not mergable, as they would require new nodes to be created. +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,14 @@ import { merge, find } from 'module'; export { find }; ``` + +There is a special case even the export is duplicate we ignore it, because it can't be merged with another import/export from the same source, it's when we have export with type export *. + +Example of **correct** code for this rule with the `{ "includeExports": true }` option: + +```js + +import { merge, find } from 'module'; + +export * from 'module'; +``` diff --git a/lib/rules/no-duplicate-imports.js b/lib/rules/no-duplicate-imports.js index 7218dc64add..1a92060698f 100644 --- a/lib/rules/no-duplicate-imports.js +++ b/lib/rules/no-duplicate-imports.js @@ -8,6 +8,42 @@ // Rule Definition //------------------------------------------------------------------------------ +const EXPORT_ALL_DECLARATION = 'ExportAllDeclaration' +const IMPORT_NAME_SPACE_SPECIFIER = 'ImportNamespaceSpecifier'; +const IMPORT_SPECIFIER = 'ImportSpecifier'; +const CONTRADICTORY_IMPORT_TYPES = [ + IMPORT_NAME_SPACE_SPECIFIER, + IMPORT_SPECIFIER, +]; + +/** + * Return the type of import. + * @param {ASTNode} node A node to get. + * @returns {string} the type of the import. + */ +function getImportType(node) { + if ( + node && + node.specifiers && + node.specifiers[0] && + node.specifiers[0].type + ) { + const index = node.specifiers.findIndex((specifier) => + CONTRADICTORY_IMPORT_TYPES.includes( + specifier.type + ) + ); + if (index > -1) { + return node.specifiers[index].type; + } else { + return node.specifiers[0].type; + } + } else if (node && node.type) { + return node.type; + } + return ""; +} + /** * Returns the name of the module imported or re-exported. * @param {ASTNode} node A node to get. @@ -21,6 +57,25 @@ function getValue(node) { return ""; } + +/** + * Returns a boolean if we should report contradictory import type. + * @param {string[]} importTypes An array contain import types of a module. + * @param {string} importType An contradictory import type to check if we should report it or not. + * @returns {boolean} true if the contradictory import type should be reported. + */ +function shouldReportContradictoryImportType(importTypes, importType) { + if (importTypes.indexOf(importType) > -1) { + return true; + } else { + return ( + importTypes.findIndex((importTypeItem) => { + return CONTRADICTORY_IMPORT_TYPES.includes(importTypeItem); + }) === -1 + ); + } +} + /** * 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. @@ -28,17 +83,43 @@ function getValue(node) { * @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 + * @param {{}} modulesWithImportTypes The object containing the name of unique modules with their first import type [specificImport, nameSpaceImport]. + * @param {string} type the name of import type. + * @param {string[]} ExportAllDeclarationsInFile The array containing ExportAllDeclarations in the file. * * @returns {void} No return value */ -function checkAndReport(context, node, value, array, messageId) { - if (array.indexOf(value) !== -1) { +function checkAndReport( + context, + node, + value, + array, + messageId, + modulesWithImportTypes, + type, + ExportAllDeclarationsInFile +) { + let isDuplicate = false; + if (type === EXPORT_ALL_DECLARATION) { + if (ExportAllDeclarationsInFile.indexOf(value) !== -1) { + isDuplicate = true; + } + } else if (array.indexOf(value) !== -1) { + isDuplicate = true; + if (CONTRADICTORY_IMPORT_TYPES.includes(type)) { + isDuplicate = shouldReportContradictoryImportType( + modulesWithImportTypes[value], + type + ); + } + } + if (isDuplicate) { context.report({ node, messageId, data: { - module: value - } + module: value, + }, }); } } @@ -54,21 +135,55 @@ function checkAndReport(context, node, value, array, messageId) { * @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 {{}} modulesWithImportTypes The object containing the name of unique modules with their first import type [specificImport, nameSpaceImport]. + * @param {string[]} ExportAllDeclarationsInFile The array containing ExportAllDeclarations in the file. * * @returns {nodeCallback} A function passed to ESLint to handle the statement. */ -function handleImports(context, includeExports, importsInFile, exportsInFile) { - return function(node) { +function handleImports( + context, + includeExports, + importsInFile, + exportsInFile, + modulesWithImportTypes, + ExportAllDeclarationsInFile +) { + return function (node) { const value = getValue(node); + const type = getImportType(node); if (value) { - checkAndReport(context, node, value, importsInFile, "import"); + checkAndReport( + context, + node, + value, + importsInFile, + "import", + modulesWithImportTypes, + type, + ExportAllDeclarationsInFile + ); if (includeExports) { - checkAndReport(context, node, value, exportsInFile, "importAs"); + checkAndReport( + context, + node, + value, + exportsInFile, + "importAs", + modulesWithImportTypes, + type, + ExportAllDeclarationsInFile + ); } - importsInFile.push(value); + if (modulesWithImportTypes[value]) { + modulesWithImportTypes[value] = modulesWithImportTypes[ + value + ].concat(type); + } else { + modulesWithImportTypes[value] = [type]; + } } }; } @@ -78,18 +193,47 @@ function handleImports(context, includeExports, importsInFile, exportsInFile) { * @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 {{}} modulesWithImportTypes The object containing the name of unique modules with their first import type [specificImport, nameSpaceImport]. + * @param {string[]} ExportAllDeclarationsInFile The array containing ExportAllDeclarations in the file. * * @returns {nodeCallback} A function passed to ESLint to handle the statement. */ -function handleExports(context, importsInFile, exportsInFile) { - return function(node) { +function handleExports( + context, + importsInFile, + exportsInFile, + modulesWithImportTypes, + ExportAllDeclarationsInFile +) { + return function (node) { const value = getValue(node); - + const type = getImportType(node); if (value) { - checkAndReport(context, node, value, exportsInFile, "export"); - checkAndReport(context, node, value, importsInFile, "exportAs"); - - exportsInFile.push(value); + checkAndReport( + context, + node, + value, + exportsInFile, + "export", + modulesWithImportTypes, + type, + ExportAllDeclarationsInFile + ); + if (type === EXPORT_ALL_DECLARATION) { + ExportAllDeclarationsInFile.push(value); + } else { + checkAndReport( + context, + node, + value, + importsInFile, + "exportAs", + modulesWithImportTypes, + type, + ExportAllDeclarationsInFile + ); + exportsInFile.push(value); + } } }; } @@ -102,41 +246,62 @@ module.exports = { description: "disallow duplicate module imports", category: "ECMAScript 6", recommended: false, - url: "https://eslint.org/docs/rules/no-duplicate-imports" + url: "https://eslint.org/docs/rules/no-duplicate-imports", }, - schema: [{ - type: "object", - properties: { - includeExports: { - type: "boolean", - default: false - } + schema: [ + { + type: "object", + properties: { + includeExports: { + type: "boolean", + default: false, + }, + }, + additionalProperties: false, }, - additionalProperties: false - }], + ], messages: { import: "'{{module}}' import is duplicated.", importAs: "'{{module}}' import is duplicated as export.", export: "'{{module}}' export is duplicated.", - exportAs: "'{{module}}' export is duplicated as import." - } + exportAs: "'{{module}}' export is duplicated as import.", + }, }, create(context) { const includeExports = (context.options[0] || {}).includeExports, importsInFile = [], - exportsInFile = []; - + modulesWithImportTypes = {}, + exportsInFile = [], + ExportAllDeclarationsInFile = []; const handlers = { - ImportDeclaration: handleImports(context, includeExports, importsInFile, exportsInFile) + ImportDeclaration: handleImports( + context, + includeExports, + importsInFile, + exportsInFile, + modulesWithImportTypes, + ExportAllDeclarationsInFile + ), }; if (includeExports) { - handlers.ExportNamedDeclaration = handleExports(context, importsInFile, exportsInFile); - handlers.ExportAllDeclaration = handleExports(context, importsInFile, exportsInFile); + handlers.ExportNamedDeclaration = handleExports( + context, + importsInFile, + exportsInFile, + modulesWithImportTypes, + ExportAllDeclarationsInFile + ); + handlers.ExportAllDeclaration = handleExports( + context, + importsInFile, + exportsInFile, + modulesWithImportTypes, + ExportAllDeclarationsInFile + ); } - return handlers; - } + }, }; diff --git a/tests/lib/rules/no-duplicate-imports.js b/tests/lib/rules/no-duplicate-imports.js index 42d35c85751..3039f47c081 100644 --- a/tests/lib/rules/no-duplicate-imports.js +++ b/tests/lib/rules/no-duplicate-imports.js @@ -16,73 +16,210 @@ const rule = require("../../../lib/rules/no-duplicate-imports"), // Tests //------------------------------------------------------------------------------ -const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 6, sourceType: "module" } }); +const ruleTester = new RuleTester({ + parserOptions: { ecmaVersion: 6, sourceType: "module" }, +}); ruleTester.run("no-duplicate-imports", rule, { valid: [ - "import os from \"os\";\nimport fs from \"fs\";", - "import { merge } from \"lodash-es\";", - "import _, { merge } from \"lodash-es\";", - "import * as Foobar from \"async\";", - "import \"foo\"", - "import os from \"os\";\nexport { something } from \"os\";", + 'import os from "os";\nimport fs from "fs";', + 'import { merge } from "lodash-es";', + 'import _, { merge } from "lodash-es";', + 'import * as Foobar from "async";', + 'import "foo"', + 'import os from "os";\nexport { something } from "os";', + 'import { something } from "os";\nimport * as foobar from "os";', + 'import foo, * as bar from "mod";\nimport { baz } from "mod";', + { + code: 'import os from "os";\nexport { hello } from "hello";', + options: [{ includeExports: true }], + }, + { + code: 'import os from "os";\nexport * from "hello";', + options: [{ includeExports: true }], + }, { - code: "import os from \"os\";\nexport { hello } from \"hello\";", - options: [{ includeExports: true }] + code: 'import os from "os";\nexport { hello as hi } from "hello";', + options: [{ includeExports: true }], }, { - code: "import os from \"os\";\nexport * from \"hello\";", - options: [{ includeExports: true }] + code: 'import os from "os";\nexport default function(){};', + options: [{ includeExports: true }], }, { - code: "import os from \"os\";\nexport { hello as hi } from \"hello\";", - options: [{ includeExports: true }] + code: + 'import { merge } from "lodash-es";\nexport { merge as lodashMerge }', + options: [{ includeExports: true }], }, + // ignore `export * from` declarations, they cannot be merged with any other import/export declarations { - code: "import os from \"os\";\nexport default function(){};", - options: [{ includeExports: true }] + code: "import os from 'os'; export * from 'os';", + options: [{ includeExports: true }], }, { - code: "import { merge } from \"lodash-es\";\nexport { merge as lodashMerge }", - options: [{ includeExports: true }] - } + code: "export * from 'os'; import { a } from 'os';", + options: [{ includeExports: true }], + }, + { + code: "import * as ns from 'os'; export * from 'os';", + options: [{ includeExports: true }], + }, + { + code: "export * from 'os'; export { a } from 'os';", + options: [{ includeExports: true }], + }, + { + code: "export { a as b } from 'os'; export * from 'os';", + options: [{ includeExports: true }], + }, ], invalid: [ { - code: "import \"fs\";\nimport \"fs\"", - errors: [{ messageId: "import", data: { module: "fs" }, type: "ImportDeclaration" }] + code: 'import "fs";\nimport "fs"', + errors: [ + { + messageId: "import", + data: { module: "fs" }, + type: "ImportDeclaration", + }, + ], }, { - code: "import { merge } from \"lodash-es\";import { find } from \"lodash-es\";", - errors: [{ messageId: "import", data: { module: "lodash-es" }, type: "ImportDeclaration" }] + code: + 'import { merge } from "lodash-es";import { find } 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", + }, + ], + }, + { + code: + 'import os from "os";\nimport * as foobar from "os";\nimport { something } from "os";', + errors: [ + { + messageId: "import", + data: { module: "os" }, + type: "ImportDeclaration", + }, + ], + }, + { + code: + 'import os from "os";\nimport * as foobar1 from "os";\nimport * as foobar2 from "os";', + errors: [ + { + messageId: "import", + data: { module: "os" }, + type: "ImportDeclaration", + }, + { + messageId: "import", + data: { module: "os" }, + type: "ImportDeclaration", + }, + ], + }, + { + code: + 'import { merge } from "lodash-es";import _ from "lodash-es";', + errors: [ + { + messageId: "import", + data: { module: "lodash-es" }, + type: "ImportDeclaration", + }, + ], + }, + { + code: + 'import foo, { merge } from "module";\nimport { baz } from "module";', + errors: [ + { + messageId: "import", + data: { module: "module" }, + type: "ImportDeclaration", + }, + ], }, { - code: "import { merge } from \"lodash-es\";import _ from \"lodash-es\";", - errors: [{ messageId: "import", data: { module: "lodash-es" }, type: "ImportDeclaration" }] + code: + 'import * as namespace from "lodash-es";\nimport { merge } from "lodash-es";\nimport { baz } from "lodash-es";', + errors: [ + { + messageId: "import", + data: { module: "lodash-es" }, + type: "ImportDeclaration", + }, + ], }, { - code: "export { os } from \"os\";\nexport { something } from \"os\";", + code: 'export { os } from "os";\nexport { something } from "os";', options: [{ includeExports: true }], - errors: [{ messageId: "export", data: { module: "os" }, type: "ExportNamedDeclaration" }] + 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"; export { os as foobar } from "os";\nexport { something } from "os";', options: [{ includeExports: true }], errors: [ - { messageId: "exportAs", data: { module: "os" }, type: "ExportNamedDeclaration" }, - { messageId: "export", data: { module: "os" }, type: "ExportNamedDeclaration" }, - { messageId: "exportAs", data: { module: "os" }, type: "ExportNamedDeclaration" } - ] + { + messageId: "exportAs", + data: { module: "os" }, + type: "ExportNamedDeclaration", + }, + { + messageId: "export", + data: { module: "os" }, + type: "ExportNamedDeclaration", + }, + { + messageId: "exportAs", + data: { module: "os" }, + type: "ExportNamedDeclaration", + }, + ], }, { - code: "import os from \"os\";\nexport { something } from \"os\";", + code: 'import os from "os";\nexport { something } from "os";', options: [{ includeExports: true }], - errors: [{ messageId: "exportAs", data: { module: "os" }, type: "ExportNamedDeclaration" }] + errors: [ + { + messageId: "exportAs", + data: { module: "os" }, + type: "ExportNamedDeclaration", + }, + ], }, { - code: "import os from \"os\";\nexport * from \"os\";", + code: "export * from 'os'; export * from 'os';", options: [{ includeExports: true }], - errors: [{ messageId: "exportAs", data: { module: "os" }, type: "ExportAllDeclaration" }] - } - ] + errors: [ + { + messageId: "export", + data: { module: "os" }, + type: "ExportAllDeclaration", + }, + ], + }, + ], });