From e20fab5a33a279fddf839a3e773f1d80ecc7ec27 Mon Sep 17 00:00:00 2001 From: soufianeboutahlil Date: Sun, 21 Mar 2021 19:22:37 +0100 Subject: [PATCH 1/9] 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", + }, + ], + }, + ], }); From 8f33a8b6a84796967340b2ea18a22ab02b37867f Mon Sep 17 00:00:00 2001 From: soufianeboutahlil Date: Sun, 21 Mar 2021 21:47:42 +0100 Subject: [PATCH 2/9] Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760) --- lib/rules/no-duplicate-imports.js | 58 ++++++------- tests/lib/rules/no-duplicate-imports.js | 107 ++++++++++++------------ 2 files changed, 83 insertions(+), 82 deletions(-) diff --git a/lib/rules/no-duplicate-imports.js b/lib/rules/no-duplicate-imports.js index 1a92060698f..0520a6e24be 100644 --- a/lib/rules/no-duplicate-imports.js +++ b/lib/rules/no-duplicate-imports.js @@ -8,12 +8,12 @@ // Rule Definition //------------------------------------------------------------------------------ -const EXPORT_ALL_DECLARATION = 'ExportAllDeclaration' -const IMPORT_NAME_SPACE_SPECIFIER = 'ImportNamespaceSpecifier'; -const IMPORT_SPECIFIER = 'ImportSpecifier'; +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, + IMPORT_SPECIFIER ]; /** @@ -28,17 +28,17 @@ function getImportType(node) { node.specifiers[0] && node.specifiers[0].type ) { - const index = node.specifiers.findIndex((specifier) => + 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.specifiers[0].type; + } + if (node && node.type) { return node.type; } return ""; @@ -67,13 +67,11 @@ function getValue(node) { function shouldReportContradictoryImportType(importTypes, importType) { if (importTypes.indexOf(importType) > -1) { return true; - } else { - return ( - importTypes.findIndex((importTypeItem) => { - return CONTRADICTORY_IMPORT_TYPES.includes(importTypeItem); - }) === -1 - ); } + return ( + importTypes.findIndex(importTypeItem => CONTRADICTORY_IMPORT_TYPES.includes(importTypeItem)) === -1 + ); + } /** @@ -100,6 +98,7 @@ function checkAndReport( ExportAllDeclarationsInFile ) { let isDuplicate = false; + if (type === EXPORT_ALL_DECLARATION) { if (ExportAllDeclarationsInFile.indexOf(value) !== -1) { isDuplicate = true; @@ -118,8 +117,8 @@ function checkAndReport( node, messageId, data: { - module: value, - }, + module: value + } }); } } @@ -148,7 +147,7 @@ function handleImports( modulesWithImportTypes, ExportAllDeclarationsInFile ) { - return function (node) { + return function(node) { const value = getValue(node); const type = getImportType(node); @@ -205,9 +204,10 @@ function handleExports( modulesWithImportTypes, ExportAllDeclarationsInFile ) { - return function (node) { + return function(node) { const value = getValue(node); const type = getImportType(node); + if (value) { checkAndReport( context, @@ -246,7 +246,7 @@ 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: [ @@ -255,18 +255,18 @@ module.exports = { properties: { includeExports: { type: "boolean", - default: false, - }, + 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) { @@ -283,7 +283,7 @@ module.exports = { exportsInFile, modulesWithImportTypes, ExportAllDeclarationsInFile - ), + ) }; if (includeExports) { @@ -303,5 +303,5 @@ module.exports = { ); } return handlers; - }, + } }; diff --git a/tests/lib/rules/no-duplicate-imports.js b/tests/lib/rules/no-duplicate-imports.js index 3039f47c081..80a99a3037c 100644 --- a/tests/lib/rules/no-duplicate-imports.js +++ b/tests/lib/rules/no-duplicate-imports.js @@ -17,7 +17,7 @@ const rule = require("../../../lib/rules/no-duplicate-imports"), //------------------------------------------------------------------------------ const ruleTester = new RuleTester({ - parserOptions: { ecmaVersion: 6, sourceType: "module" }, + parserOptions: { ecmaVersion: 6, sourceType: "module" } }); ruleTester.run("no-duplicate-imports", rule, { @@ -32,46 +32,47 @@ ruleTester.run("no-duplicate-imports", rule, { 'import foo, * as bar from "mod";\nimport { baz } from "mod";', { code: 'import os from "os";\nexport { hello } from "hello";', - options: [{ includeExports: true }], + options: [{ includeExports: true }] }, { code: 'import os from "os";\nexport * from "hello";', - options: [{ includeExports: true }], + options: [{ includeExports: true }] }, { code: 'import os from "os";\nexport { hello as hi } from "hello";', - options: [{ includeExports: true }], + options: [{ includeExports: true }] }, { code: 'import os from "os";\nexport default function(){};', - options: [{ includeExports: true }], + options: [{ includeExports: true }] }, { code: 'import { merge } from "lodash-es";\nexport { merge as lodashMerge }', - options: [{ includeExports: true }], + options: [{ includeExports: true }] }, + // ignore `export * from` declarations, they cannot be merged with any other import/export declarations { code: "import os from 'os'; export * from 'os';", - options: [{ includeExports: true }], + options: [{ includeExports: true }] }, { code: "export * from 'os'; import { a } from 'os';", - options: [{ includeExports: true }], + options: [{ includeExports: true }] }, { code: "import * as ns from 'os'; export * from 'os';", - options: [{ includeExports: true }], + options: [{ includeExports: true }] }, { code: "export * from 'os'; export { a } from 'os';", - options: [{ includeExports: true }], + options: [{ includeExports: true }] }, { code: "export { a as b } from 'os'; export * from 'os';", - options: [{ includeExports: true }], - }, + options: [{ includeExports: true }] + } ], invalid: [ { @@ -80,9 +81,9 @@ ruleTester.run("no-duplicate-imports", rule, { { messageId: "import", data: { module: "fs" }, - type: "ImportDeclaration", - }, - ], + type: "ImportDeclaration" + } + ] }, { code: @@ -91,9 +92,9 @@ ruleTester.run("no-duplicate-imports", rule, { { messageId: "import", data: { module: "lodash-es" }, - type: "ImportDeclaration", - }, - ], + type: "ImportDeclaration" + } + ] }, { code: @@ -102,9 +103,9 @@ ruleTester.run("no-duplicate-imports", rule, { { messageId: "import", data: { module: "os" }, - type: "ImportDeclaration", - }, - ], + type: "ImportDeclaration" + } + ] }, { code: @@ -113,9 +114,9 @@ ruleTester.run("no-duplicate-imports", rule, { { messageId: "import", data: { module: "os" }, - type: "ImportDeclaration", - }, - ], + type: "ImportDeclaration" + } + ] }, { code: @@ -124,14 +125,14 @@ ruleTester.run("no-duplicate-imports", rule, { { messageId: "import", data: { module: "os" }, - type: "ImportDeclaration", + type: "ImportDeclaration" }, { messageId: "import", data: { module: "os" }, - type: "ImportDeclaration", - }, - ], + type: "ImportDeclaration" + } + ] }, { code: @@ -140,9 +141,9 @@ ruleTester.run("no-duplicate-imports", rule, { { messageId: "import", data: { module: "lodash-es" }, - type: "ImportDeclaration", - }, - ], + type: "ImportDeclaration" + } + ] }, { code: @@ -151,9 +152,9 @@ ruleTester.run("no-duplicate-imports", rule, { { messageId: "import", data: { module: "module" }, - type: "ImportDeclaration", - }, - ], + type: "ImportDeclaration" + } + ] }, { code: @@ -162,9 +163,9 @@ ruleTester.run("no-duplicate-imports", rule, { { messageId: "import", data: { module: "lodash-es" }, - type: "ImportDeclaration", - }, - ], + type: "ImportDeclaration" + } + ] }, { code: 'export { os } from "os";\nexport { something } from "os";', @@ -173,9 +174,9 @@ ruleTester.run("no-duplicate-imports", rule, { { messageId: "export", data: { module: "os" }, - type: "ExportNamedDeclaration", - }, - ], + type: "ExportNamedDeclaration" + } + ] }, { code: @@ -185,19 +186,19 @@ ruleTester.run("no-duplicate-imports", rule, { { messageId: "exportAs", data: { module: "os" }, - type: "ExportNamedDeclaration", + type: "ExportNamedDeclaration" }, { messageId: "export", data: { module: "os" }, - type: "ExportNamedDeclaration", + type: "ExportNamedDeclaration" }, { messageId: "exportAs", data: { module: "os" }, - type: "ExportNamedDeclaration", - }, - ], + type: "ExportNamedDeclaration" + } + ] }, { code: 'import os from "os";\nexport { something } from "os";', @@ -206,9 +207,9 @@ ruleTester.run("no-duplicate-imports", rule, { { messageId: "exportAs", data: { module: "os" }, - type: "ExportNamedDeclaration", - }, - ], + type: "ExportNamedDeclaration" + } + ] }, { code: "export * from 'os'; export * from 'os';", @@ -217,9 +218,9 @@ ruleTester.run("no-duplicate-imports", rule, { { messageId: "export", data: { module: "os" }, - type: "ExportAllDeclaration", - }, - ], - }, - ], + type: "ExportAllDeclaration" + } + ] + } + ] }); From 2a54321e8b71fc10df2665797d6eb1c307ea285d Mon Sep 17 00:00:00 2001 From: soufianeboutahlil Date: Tue, 23 Mar 2021 20:59:55 +0100 Subject: [PATCH 3/9] 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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rules/no-duplicate-imports.md b/docs/rules/no-duplicate-imports.md index ea198721a0d..ac536dd5922 100644 --- a/docs/rules/no-duplicate-imports.md +++ b/docs/rules/no-duplicate-imports.md @@ -70,7 +70,7 @@ 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 From 1932d2c87ad2de764aca9b0feb2e883b592c2b3e Mon Sep 17 00:00:00 2001 From: soufianeboutahlil Date: Sun, 9 May 2021 17:24:22 +0000 Subject: [PATCH 4/9] Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760) --- lib/rules/no-duplicate-imports.js | 357 ++++++++++++------------ tests/lib/rules/no-duplicate-imports.js | 73 +++-- 2 files changed, 215 insertions(+), 215 deletions(-) diff --git a/lib/rules/no-duplicate-imports.js b/lib/rules/no-duplicate-imports.js index 0520a6e24be..62174e55dfd 100644 --- a/lib/rules/no-duplicate-imports.js +++ b/lib/rules/no-duplicate-imports.js @@ -8,119 +8,188 @@ // 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 -]; +/** + * Check if (import|export) type is belong to (ImportSpecifier|ExportSpecifier) or (ImportNamespaceSpecifier|ExportNamespaceSpecifier). + * @param {string} importExportType An import type to get. + * @param {string} type Specifier type can be namespace or specifier + * @returns {boolean} True if (import|export) type is belong to (ImportSpecifier|ExportSpecifier) or (ImportNamespaceSpecifier|ExportNamespaceSpecifier) and false if it doesn't. + */ +function isImportExportSpecifier(importExportType, type) { + const specifiersTypes = ["ImportSpecifier", "ExportSpecifier"]; + const namespacesTypes = [ + "ImportNamespaceSpecifier", + "ExportNamespaceSpecifier" + ]; + const arrayToCheck = + type === "specifier" ? specifiersTypes : namespacesTypes; + + return arrayToCheck.includes(importExportType); +} /** - * Return the type of import. + * Return the type of (import|export). * @param {ASTNode} node A node to get. - * @returns {string} the type of the import. + * @returns {string} The type of the (import|export). */ -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 - )); +function getImportExportType(node) { + if (node && node.specifiers && node.specifiers.length > 0) { + const nodeSpecifiers = node.specifiers; + const index = nodeSpecifiers.findIndex( + ({ type }) => + isImportExportSpecifier(type, "specifier") || + isImportExportSpecifier(type, "namespace") + ); + const i = index > -1 ? index : 0; - if (index > -1) { - return node.specifiers[index].type; + return nodeSpecifiers[i].type; + } + if (node.type === "ExportAllDeclaration") { + if (node.exported) { + return "ExportNamespaceSpecifier"; } - return node.specifiers[0].type; + return "ExportAll"; } - if (node && node.type) { - return node.type; + if (node.type === "ImportDeclaration") { + return "SideEffectImport"; } - return ""; + return node.type; } /** - * 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. + * Returns a boolean indicates if two (import|export) can be merged + * @param {ASTNode} node1 A node to get. + * @param {ASTNode} node2 A node to get. + * @returns {boolean} True if two (import|export) can be merged, false if they can't. */ -function getValue(node) { - if (node && node.source && node.source.value) { - return node.source.value.trim(); - } +function isImportExportCanBeMerged(node1, node2) { + const importExportType1 = getImportExportType(node1); + const importExportType2 = getImportExportType(node2); - return ""; + if ( + (importExportType1 === "ExportAll" && + importExportType2 !== "ExportAll" && + importExportType2 !== "SideEffectImport") || + (importExportType1 !== "ExportAll" && + importExportType1 !== "SideEffectImport" && + importExportType2 === "ExportAll") + ) { + return false; + } + if ( + (isImportExportSpecifier(importExportType1, "namespace") && + isImportExportSpecifier(importExportType2, "specifier")) || + (isImportExportSpecifier(importExportType2, "namespace") && + isImportExportSpecifier(importExportType1, "specifier")) + ) { + return false; + } + return true; } +/** + * 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 shouldReportImportExport(node, previousNodes) { + if (previousNodes) { + let i = 0; + + while (i < previousNodes.length) { + if (isImportExportCanBeMerged(node, previousNodes[i])) { + return true; + } + i++; + } + } + return false; +} /** - * 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. + * 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, if the nodes are null we return []. */ -function shouldReportContradictoryImportType(importTypes, importType) { - if (importTypes.indexOf(importType) > -1) { - return true; +function getNodesByDeclarationTypeAndFormat(nodes, type) { + if (nodes) { + return nodes + .filter(({ declarationType }) => declarationType === type) + .map(({ node }) => node); } - return ( - importTypes.findIndex(importTypeItem => CONTRADICTORY_IMPORT_TYPES.includes(importTypeItem)) === -1 - ); + return []; +} +/** + * 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 ""; } /** - * Checks if the name of the import or export exists in the given array, and reports if so. + * Checks if the (import|export) can be merged with at least one import and one export, 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 - * @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 + * @param {{string: [{node: ASTNode, declarationType: string}]}} modules An 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 {void} No return value. */ function checkAndReport( context, node, - value, - array, - messageId, - modulesWithImportTypes, - type, - ExportAllDeclarationsInFile + modules, + declarationType, + includeExports ) { - let isDuplicate = false; + const module = getModule(node); + const previousNodes = modules[module]; + const messagesIds = []; + const importNodes = getNodesByDeclarationTypeAndFormat( + previousNodes, + "import" + ); + let exportNodes; - if (type === EXPORT_ALL_DECLARATION) { - if (ExportAllDeclarationsInFile.indexOf(value) !== -1) { - isDuplicate = true; + if (includeExports) { + exportNodes = getNodesByDeclarationTypeAndFormat( + previousNodes, + "export" + ); + } + if (declarationType === "import") { + if (shouldReportImportExport(node, importNodes)) { + messagesIds.push("import"); } - } else if (array.indexOf(value) !== -1) { - isDuplicate = true; - if (CONTRADICTORY_IMPORT_TYPES.includes(type)) { - isDuplicate = shouldReportContradictoryImportType( - modulesWithImportTypes[value], - type - ); + if (includeExports) { + if (shouldReportImportExport(node, exportNodes)) { + messagesIds.push("exportAs"); + } + } + } else if (declarationType === "export") { + if (shouldReportImportExport(node, exportNodes)) { + messagesIds.push("export"); + } + if (shouldReportImportExport(node, importNodes)) { + messagesIds.push("importAs"); } } - if (isDuplicate) { + + messagesIds.forEach(messageId => context.report({ node, messageId, data: { - module: value + module } - }); - } + })); } /** @@ -129,110 +198,37 @@ function checkAndReport( */ /** - * Returns a function handling the imports of a given file + * Returns a function handling the (imports|exports) of a given file * @param {RuleContext} context The ESLint rule context object. + * @param {{string: [{node: ASTNode, declarationType: string}]}} modules An 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. - * @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( +function handleImportsExports( context, - includeExports, - importsInFile, - exportsInFile, - modulesWithImportTypes, - ExportAllDeclarationsInFile + modules, + declarationType, + includeExports ) { return function(node) { - const value = getValue(node); - const type = getImportType(node); + const module = getModule(node); - if (value) { + if (module) { checkAndReport( context, node, - value, - importsInFile, - "import", - modulesWithImportTypes, - type, - ExportAllDeclarationsInFile + modules, + declarationType, + includeExports ); + const previousNodes = modules[module]; + const currentNode = { node, declarationType }; - if (includeExports) { - checkAndReport( - context, - node, - value, - exportsInFile, - "importAs", - modulesWithImportTypes, - type, - ExportAllDeclarationsInFile - ); - } - importsInFile.push(value); - if (modulesWithImportTypes[value]) { - modulesWithImportTypes[value] = modulesWithImportTypes[ - value - ].concat(type); + if (previousNodes) { + modules[module] = [...previousNodes, currentNode]; } else { - modulesWithImportTypes[value] = [type]; - } - } - }; -} - -/** - * 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 {{}} 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, - modulesWithImportTypes, - ExportAllDeclarationsInFile -) { - return function(node) { - const value = getValue(node); - const type = getImportType(node); - - if (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); + modules[module] = [currentNode]; } } }; @@ -271,35 +267,28 @@ module.exports = { create(context) { const includeExports = (context.options[0] || {}).includeExports, - importsInFile = [], - modulesWithImportTypes = {}, - exportsInFile = [], - ExportAllDeclarationsInFile = []; + modules = {}; const handlers = { - ImportDeclaration: handleImports( + ImportDeclaration: handleImportsExports( context, - includeExports, - importsInFile, - exportsInFile, - modulesWithImportTypes, - ExportAllDeclarationsInFile + modules, + "import", + includeExports ) }; if (includeExports) { - handlers.ExportNamedDeclaration = handleExports( + handlers.ExportNamedDeclaration = handleImportsExports( context, - importsInFile, - exportsInFile, - modulesWithImportTypes, - ExportAllDeclarationsInFile + modules, + "export", + includeExports ); - handlers.ExportAllDeclaration = handleExports( + handlers.ExportAllDeclaration = handleImportsExports( context, - importsInFile, - exportsInFile, - modulesWithImportTypes, - ExportAllDeclarationsInFile + modules, + "export", + includeExports ); } return handlers; diff --git a/tests/lib/rules/no-duplicate-imports.js b/tests/lib/rules/no-duplicate-imports.js index 80a99a3037c..d81ce3185d4 100644 --- a/tests/lib/rules/no-duplicate-imports.js +++ b/tests/lib/rules/no-duplicate-imports.js @@ -17,7 +17,7 @@ const rule = require("../../../lib/rules/no-duplicate-imports"), //------------------------------------------------------------------------------ const ruleTester = new RuleTester({ - parserOptions: { ecmaVersion: 6, sourceType: "module" } + parserOptions: { ecmaVersion: 12, sourceType: "module" } }); ruleTester.run("no-duplicate-imports", rule, { @@ -34,10 +34,6 @@ ruleTester.run("no-duplicate-imports", rule, { 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 as hi } from "hello";', options: [{ includeExports: true }] @@ -48,29 +44,27 @@ ruleTester.run("no-duplicate-imports", rule, { }, { code: - 'import { merge } from "lodash-es";\nexport { merge as lodashMerge }', + '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'; export * from 'os';", + code: "import * as foo from 'os';\nexport {too} from 'os';", options: [{ includeExports: true }] }, { - code: "export * from 'os'; import { a } from 'os';", + code: "import os from 'os';\n export * from 'os';", options: [{ includeExports: true }] }, { - code: "import * as ns from 'os'; export * from 'os';", + code: "import * as ns from 'os';\nexport * from 'os';", options: [{ includeExports: true }] }, { - code: "export * from 'os'; export { a } from 'os';", + code: "export * from 'os';\nexport { a } from 'os';", options: [{ includeExports: true }] }, { - code: "export { a as b } from 'os'; export * from 'os';", + code: "export { a as b } from 'os';\nexport * from 'os';", options: [{ includeExports: true }] } ], @@ -87,7 +81,7 @@ ruleTester.run("no-duplicate-imports", rule, { }, { code: - 'import { merge } from "lodash-es";import { find } from "lodash-es";', + 'import { merge } from "lodash-es";\nimport { find } from "lodash-es";', errors: [ { messageId: "import", @@ -98,19 +92,13 @@ ruleTester.run("no-duplicate-imports", rule, { }, { code: - 'import os from "os";\nimport { something } from "os";\nimport * as foobar from "os";', + '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" }, @@ -120,7 +108,7 @@ ruleTester.run("no-duplicate-imports", rule, { }, { code: - 'import os from "os";\nimport * as foobar1 from "os";\nimport * as foobar2 from "os";', + 'import os from "os";\nimport * as foobar1 from "os";\nimport * as foobar2 from "os";', errors: [ { messageId: "import", @@ -136,7 +124,7 @@ ruleTester.run("no-duplicate-imports", rule, { }, { code: - 'import { merge } from "lodash-es";import _ from "lodash-es";', + 'import { merge } from "lodash-es";\nimport _ from "lodash-es";', errors: [ { messageId: "import", @@ -147,7 +135,7 @@ ruleTester.run("no-duplicate-imports", rule, { }, { code: - 'import foo, { merge } from "module";\nimport { baz } from "module";', + 'import foo, { merge } from "module";\nimport { baz } from "module";', errors: [ { messageId: "import", @@ -158,7 +146,7 @@ ruleTester.run("no-duplicate-imports", rule, { }, { code: - 'import * as namespace from "lodash-es";\nimport { merge } from "lodash-es";\nimport { baz } from "lodash-es";', + 'import * as modns from "lodash-es";\nimport { merge } from "lodash-es";\nimport { baz } from "lodash-es";', errors: [ { messageId: "import", @@ -180,11 +168,11 @@ ruleTester.run("no-duplicate-imports", rule, { }, { code: - 'import os from "os"; export { os as foobar } from "os";\nexport { something } from "os";', + 'import os from "os"; export { os as foobar } from "os";\nexport { something } from "os";', options: [{ includeExports: true }], errors: [ { - messageId: "exportAs", + messageId: "importAs", data: { module: "os" }, type: "ExportNamedDeclaration" }, @@ -194,7 +182,7 @@ ruleTester.run("no-duplicate-imports", rule, { type: "ExportNamedDeclaration" }, { - messageId: "exportAs", + messageId: "importAs", data: { module: "os" }, type: "ExportNamedDeclaration" } @@ -205,14 +193,14 @@ ruleTester.run("no-duplicate-imports", rule, { options: [{ includeExports: true }], errors: [ { - messageId: "exportAs", + messageId: "importAs", data: { module: "os" }, type: "ExportNamedDeclaration" } ] }, { - code: "export * from 'os'; export * from 'os';", + code: "export * from 'os';\nexport * from 'os';", options: [{ includeExports: true }], errors: [ { @@ -221,6 +209,29 @@ ruleTester.run("no-duplicate-imports", rule, { type: "ExportAllDeclaration" } ] + }, + { + code: "import 'os';\nexport * from 'os';", + options: [{ includeExports: true }], + errors: [ + { + messageId: "importAs", + data: { module: "os" }, + type: "ExportAllDeclaration" + } + ] + }, + { + code: + "import * as modns from 'mod';\nexport * as modns from 'mod';", + options: [{ includeExports: true }], + errors: [ + { + messageId: "importAs", + data: { module: "mod" }, + type: "ExportAllDeclaration" + } + ] } ] }); From 7bb7a50e71cc003b6d1a112c508471eae8f1a92a Mon Sep 17 00:00:00 2001 From: soufianeboutahlil Date: Sun, 23 May 2021 14:43:37 +0100 Subject: [PATCH 5/9] Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760) --- lib/rules/no-duplicate-imports.js | 137 ++++++++++++++---------------- 1 file changed, 63 insertions(+), 74 deletions(-) diff --git a/lib/rules/no-duplicate-imports.js b/lib/rules/no-duplicate-imports.js index 62174e55dfd..9d7e2019f55 100644 --- a/lib/rules/no-duplicate-imports.js +++ b/lib/rules/no-duplicate-imports.js @@ -15,13 +15,12 @@ * @returns {boolean} True if (import|export) type is belong to (ImportSpecifier|ExportSpecifier) or (ImportNamespaceSpecifier|ExportNamespaceSpecifier) and false if it doesn't. */ function isImportExportSpecifier(importExportType, type) { - const specifiersTypes = ["ImportSpecifier", "ExportSpecifier"]; + const namedTypes = ["ImportSpecifier", "ExportSpecifier"]; const namespacesTypes = [ "ImportNamespaceSpecifier", "ExportNamespaceSpecifier" ]; - const arrayToCheck = - type === "specifier" ? specifiersTypes : namespacesTypes; + const arrayToCheck = type === "named" ? namedTypes : namespacesTypes; return arrayToCheck.includes(importExportType); } @@ -32,12 +31,12 @@ function isImportExportSpecifier(importExportType, type) { * @returns {string} The type of the (import|export). */ function getImportExportType(node) { - if (node && node.specifiers && node.specifiers.length > 0) { + if (node.specifiers && node.specifiers.length > 0) { const nodeSpecifiers = node.specifiers; const index = nodeSpecifiers.findIndex( ({ type }) => - isImportExportSpecifier(type, "specifier") || - isImportExportSpecifier(type, "namespace") + isImportExportSpecifier(type, "named") || + isImportExportSpecifier(type, "namespace") ); const i = index > -1 ? index : 0; @@ -49,10 +48,7 @@ function getImportExportType(node) { } return "ExportAll"; } - if (node.type === "ImportDeclaration") { - return "SideEffectImport"; - } - return node.type; + return "SideEffectImport"; } /** @@ -67,19 +63,19 @@ function isImportExportCanBeMerged(node1, node2) { if ( (importExportType1 === "ExportAll" && - importExportType2 !== "ExportAll" && - importExportType2 !== "SideEffectImport") || - (importExportType1 !== "ExportAll" && - importExportType1 !== "SideEffectImport" && - importExportType2 === "ExportAll") + importExportType2 !== "ExportAll" && + importExportType2 !== "SideEffectImport") || + (importExportType1 !== "ExportAll" && + importExportType1 !== "SideEffectImport" && + importExportType2 === "ExportAll") ) { return false; } if ( (isImportExportSpecifier(importExportType1, "namespace") && - isImportExportSpecifier(importExportType2, "specifier")) || - (isImportExportSpecifier(importExportType2, "namespace") && - isImportExportSpecifier(importExportType1, "specifier")) + isImportExportSpecifier(importExportType2, "named")) || + (isImportExportSpecifier(importExportType2, "namespace") && + isImportExportSpecifier(importExportType1, "named")) ) { return false; } @@ -93,15 +89,13 @@ function isImportExportCanBeMerged(node1, node2) { * @returns {boolean} True if the (import/export) should be reported. */ function shouldReportImportExport(node, previousNodes) { - if (previousNodes) { - let i = 0; + let i = 0; - while (i < previousNodes.length) { - if (isImportExportCanBeMerged(node, previousNodes[i])) { - return true; - } - i++; + while (i < previousNodes.length) { + if (isImportExportCanBeMerged(node, previousNodes[i])) { + return true; } + i++; } return false; } @@ -112,13 +106,10 @@ function shouldReportImportExport(node, previousNodes) { * @param {string} type Declaration type. * @returns {[ASTNode]} An array contains only nodes with declarations types equal to type, if the nodes are null we return []. */ -function getNodesByDeclarationTypeAndFormat(nodes, type) { - if (nodes) { - return nodes - .filter(({ declarationType }) => declarationType === type) - .map(({ node }) => node); - } - return []; +function getNodesByDeclarationType(nodes, type) { + return nodes + .filter(({ declarationType }) => declarationType === type) + .map(({ node }) => node); } /** @@ -137,7 +128,7 @@ function getModule(node) { * Checks if the (import|export) can be merged with at least one import and one export, and reports if so. * @param {RuleContext} context The ESLint rule context object. * @param {ASTNode} node A node to get. - * @param {{string: [{node: ASTNode, declarationType: string}]}} modules An 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 {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 {void} No return value. @@ -150,46 +141,42 @@ function checkAndReport( includeExports ) { const module = getModule(node); - const previousNodes = modules[module]; - const messagesIds = []; - const importNodes = getNodesByDeclarationTypeAndFormat( - previousNodes, - "import" - ); - let exportNodes; - if (includeExports) { - exportNodes = getNodesByDeclarationTypeAndFormat( - previousNodes, - "export" - ); - } - if (declarationType === "import") { - if (shouldReportImportExport(node, importNodes)) { - messagesIds.push("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) { + if (shouldReportImportExport(node, exportNodes)) { + messagesIds.push("exportAs"); + } + } + } else if (declarationType === "export") { if (shouldReportImportExport(node, exportNodes)) { - messagesIds.push("exportAs"); + messagesIds.push("export"); + } + if (shouldReportImportExport(node, importNodes)) { + messagesIds.push("importAs"); } } - } else if (declarationType === "export") { - if (shouldReportImportExport(node, exportNodes)) { - messagesIds.push("export"); - } - if (shouldReportImportExport(node, importNodes)) { - messagesIds.push("importAs"); - } + messagesIds.forEach(messageId => + context.report({ + node, + messageId, + data: { + module + } + })); } - - messagesIds.forEach(messageId => - context.report({ - node, - messageId, - data: { - module - } - })); } /** @@ -200,7 +187,7 @@ function checkAndReport( /** * Returns a function handling the (imports|exports) of a given file * @param {RuleContext} context The ESLint rule context object. - * @param {{string: [{node: ASTNode, declarationType: string}]}} modules An 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 {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. @@ -222,14 +209,15 @@ function handleImportsExports( declarationType, includeExports ); - const previousNodes = modules[module]; const currentNode = { node, declarationType }; + let nodes = [currentNode]; - if (previousNodes) { - modules[module] = [...previousNodes, currentNode]; - } else { - modules[module] = [currentNode]; + if (modules.has(module)) { + const previousNodes = modules.get(module); + + nodes = [...previousNodes, currentNode]; } + modules.set(module, nodes); } }; } @@ -257,6 +245,7 @@ module.exports = { additionalProperties: false } ], + messages: { import: "'{{module}}' import is duplicated.", importAs: "'{{module}}' import is duplicated as export.", @@ -267,7 +256,7 @@ module.exports = { create(context) { const includeExports = (context.options[0] || {}).includeExports, - modules = {}; + modules = new Map(); const handlers = { ImportDeclaration: handleImportsExports( context, From 9d3fd4d18d977181a323eef6df33a8982a997738 Mon Sep 17 00:00:00 2001 From: soufianeboutahlil Date: Thu, 27 May 2021 00:58:00 +0100 Subject: [PATCH 6/9] Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760) --- lib/rules/no-duplicate-imports.js | 21 ++- tests/lib/rules/no-duplicate-imports.js | 203 +++++------------------- 2 files changed, 53 insertions(+), 171 deletions(-) diff --git a/lib/rules/no-duplicate-imports.js b/lib/rules/no-duplicate-imports.js index 9d7e2019f55..c1c1972bc4c 100644 --- a/lib/rules/no-duplicate-imports.js +++ b/lib/rules/no-duplicate-imports.js @@ -4,6 +4,16 @@ */ "use strict"; +//------------------------------------------------------------------------------ +// Helpers +//------------------------------------------------------------------------------ + +const NAMED_TYPES = ["ImportSpecifier", "ExportSpecifier"]; +const NAMESPACE_TYPES = [ + "ImportNamespaceSpecifier", + "ExportNamespaceSpecifier" +]; + //------------------------------------------------------------------------------ // Rule Definition //------------------------------------------------------------------------------ @@ -15,12 +25,7 @@ * @returns {boolean} True if (import|export) type is belong to (ImportSpecifier|ExportSpecifier) or (ImportNamespaceSpecifier|ExportNamespaceSpecifier) and false if it doesn't. */ function isImportExportSpecifier(importExportType, type) { - const namedTypes = ["ImportSpecifier", "ExportSpecifier"]; - const namespacesTypes = [ - "ImportNamespaceSpecifier", - "ExportNamespaceSpecifier" - ]; - const arrayToCheck = type === "named" ? namedTypes : namespacesTypes; + const arrayToCheck = type === "named" ? NAMED_TYPES : NAMESPACE_TYPES; return arrayToCheck.includes(importExportType); } @@ -157,7 +162,7 @@ function checkAndReport( } if (includeExports) { if (shouldReportImportExport(node, exportNodes)) { - messagesIds.push("exportAs"); + messagesIds.push("importAs"); } } } else if (declarationType === "export") { @@ -165,7 +170,7 @@ function checkAndReport( messagesIds.push("export"); } if (shouldReportImportExport(node, importNodes)) { - messagesIds.push("importAs"); + messagesIds.push("exportAs"); } } messagesIds.forEach(messageId => diff --git a/tests/lib/rules/no-duplicate-imports.js b/tests/lib/rules/no-duplicate-imports.js index d81ce3185d4..7da472cce27 100644 --- a/tests/lib/rules/no-duplicate-imports.js +++ b/tests/lib/rules/no-duplicate-imports.js @@ -16,222 +16,99 @@ const rule = require("../../../lib/rules/no-duplicate-imports"), // Tests //------------------------------------------------------------------------------ -const ruleTester = new RuleTester({ - parserOptions: { ecmaVersion: 12, 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 { 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";', + "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 * as bar from \"os\";\nimport { baz } from \"os\";", + "import foo, * as bar from \"os\";\nimport { baz } from \"os\";", + { + code: "import os from \"os\";\nexport { hello } from \"hello\";", options: [{ includeExports: true }] }, { - code: 'import os from "os";\nexport { hello as hi } from "hello";', + code: "import os from \"os\";\nexport * from \"hello\";", options: [{ includeExports: true }] }, { - code: 'import os from "os";\nexport default function(){};', + code: "import os from \"os\";\nexport { hello as hi } from \"hello\";", options: [{ includeExports: true }] }, { - code: - 'import { merge } from "lodash-es";\nexport { merge as lodashMerge }', + code: "import os from \"os\";\nexport default function(){};", options: [{ includeExports: true }] }, { - code: "import * as foo from 'os';\nexport {too} from 'os';", + code: "import { merge } from \"lodash-es\";\nexport { merge as lodashMerge }", options: [{ includeExports: true }] }, { - code: "import os from 'os';\n export * from 'os';", + code: "import os from \"os\";\nexport * from \"os\";", options: [{ includeExports: true }] }, { - code: "import * as ns from 'os';\nexport * from 'os';", - options: [{ includeExports: true }] - }, - { - code: "export * from 'os';\nexport { a } from 'os';", - options: [{ includeExports: true }] - }, - { - code: "export { a as b } from 'os';\nexport * from 'os';", + code: "export { something } from \"os\";\nexport * from \"os\";", options: [{ includeExports: true }] } ], invalid: [ { - code: 'import "fs";\nimport "fs"', - errors: [ - { - messageId: "import", - data: { module: "fs" }, - type: "ImportDeclaration" - } - ] - }, - { - code: - 'import { merge } from "lodash-es";\nimport { 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" - }, - { - 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 \"fs\";\nimport \"fs\"", + errors: [{ messageId: "import", data: { module: "fs" }, type: "ImportDeclaration" }] }, { - code: - 'import { merge } from "lodash-es";\nimport _ from "lodash-es";', - errors: [ - { - messageId: "import", - data: { module: "lodash-es" }, - type: "ImportDeclaration" - } - ] + code: "import { merge } from \"lodash-es\";\nimport { find } 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\";\nimport _ from \"lodash-es\";", + errors: [{ messageId: "import", data: { module: "lodash-es" }, type: "ImportDeclaration" }] }, { - code: - 'import * as modns from "lodash-es";\nimport { merge } from "lodash-es";\nimport { baz } from "lodash-es";', + code: "import os from \"os\";\nimport { something } from \"os\";\nimport * as foobar from \"os\";", errors: [ - { - messageId: "import", - data: { module: "lodash-es" }, - type: "ImportDeclaration" - } + { messageId: "import", data: { module: "os" }, type: "ImportDeclaration" }, + { messageId: "import", data: { module: "os" }, type: "ImportDeclaration" } ] }, { - code: 'export { os } from "os";\nexport { something } from "os";', - options: [{ includeExports: true }], - errors: [ - { - messageId: "export", - data: { module: "os" }, - type: "ExportNamedDeclaration" - } - ] + 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" }] }, { - code: - 'import os from "os"; export { os as foobar } from "os";\nexport { something } from "os";', + code: "export { os } from \"os\";\nexport { something } from \"os\";", options: [{ includeExports: true }], - errors: [ - { - messageId: "importAs", - data: { module: "os" }, - type: "ExportNamedDeclaration" - }, - { - messageId: "export", - data: { module: "os" }, - type: "ExportNamedDeclaration" - }, - { - messageId: "importAs", - data: { module: "os" }, - type: "ExportNamedDeclaration" - } - ] + errors: [{ messageId: "export", data: { module: "os" }, type: "ExportNamedDeclaration" }] }, { - code: 'import os 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: "importAs", - 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: "export * from 'os';\nexport * from 'os';", + code: "import os from \"os\";\nexport { something } from \"os\";", options: [{ includeExports: true }], - errors: [ - { - messageId: "export", - data: { module: "os" }, - type: "ExportAllDeclaration" - } - ] + errors: [{ messageId: "exportAs", data: { module: "os" }, type: "ExportNamedDeclaration" }] }, { - code: "import 'os';\nexport * from 'os';", + code: "export * from \"os\";\nexport * from \"os\";", options: [{ includeExports: true }], - errors: [ - { - messageId: "importAs", - data: { module: "os" }, - type: "ExportAllDeclaration" - } - ] + errors: [{ messageId: "export", data: { module: "os" }, type: "ExportAllDeclaration" }] }, { - code: - "import * as modns from 'mod';\nexport * as modns from 'mod';", + code: "import \"os\";\nexport * from \"os\";", options: [{ includeExports: true }], - errors: [ - { - messageId: "importAs", - data: { module: "mod" }, - type: "ExportAllDeclaration" - } - ] + errors: [{ messageId: "exportAs", data: { module: "os" }, type: "ExportAllDeclaration" }] } ] }); From f97848b86bcef670566939c5688d740d9b60354f Mon Sep 17 00:00:00 2001 From: soufianeboutahlil Date: Thu, 27 May 2021 01:07:25 +0100 Subject: [PATCH 7/9] Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760) --- tests/lib/rules/no-duplicate-imports.js | 34 +++---------------------- 1 file changed, 4 insertions(+), 30 deletions(-) diff --git a/tests/lib/rules/no-duplicate-imports.js b/tests/lib/rules/no-duplicate-imports.js index 7da472cce27..42d35c85751 100644 --- a/tests/lib/rules/no-duplicate-imports.js +++ b/tests/lib/rules/no-duplicate-imports.js @@ -26,8 +26,6 @@ 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\";", { code: "import os from \"os\";\nexport { hello } from \"hello\";", options: [{ includeExports: true }] @@ -47,14 +45,6 @@ ruleTester.run("no-duplicate-imports", rule, { { code: "import { merge } from \"lodash-es\";\nexport { merge as lodashMerge }", 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: [ @@ -63,22 +53,11 @@ ruleTester.run("no-duplicate-imports", rule, { errors: [{ messageId: "import", data: { module: "fs" }, type: "ImportDeclaration" }] }, { - 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\";\nimport _ from \"lodash-es\";", + 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" }, - { messageId: "import", data: { module: "os" }, type: "ImportDeclaration" } - ] - }, - { - code: "import * as modns from \"lodash-es\";\nimport { merge } from \"lodash-es\";\nimport { baz } from \"lodash-es\";", + code: "import { merge } from \"lodash-es\";import _ from \"lodash-es\";", errors: [{ messageId: "import", data: { module: "lodash-es" }, type: "ImportDeclaration" }] }, { @@ -87,7 +66,7 @@ ruleTester.run("no-duplicate-imports", rule, { errors: [{ messageId: "export", data: { module: "os" }, type: "ExportNamedDeclaration" }] }, { - code: "import os from \"os\";\nexport { 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" }, @@ -101,12 +80,7 @@ ruleTester.run("no-duplicate-imports", rule, { errors: [{ messageId: "exportAs", data: { module: "os" }, type: "ExportNamedDeclaration" }] }, { - code: "export * from \"os\";\nexport * from \"os\";", - options: [{ includeExports: true }], - errors: [{ messageId: "export", data: { module: "os" }, type: "ExportAllDeclaration" }] - }, - { - code: "import \"os\";\nexport * from \"os\";", + code: "import os from \"os\";\nexport * from \"os\";", options: [{ includeExports: true }], errors: [{ messageId: "exportAs", data: { module: "os" }, type: "ExportAllDeclaration" }] } From d515f8d82856cceaad40c1413c79808d936e25c0 Mon Sep 17 00:00:00 2001 From: soufianeboutahlil Date: Thu, 27 May 2021 01:20:50 +0100 Subject: [PATCH 8/9] Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760) --- tests/lib/rules/no-duplicate-imports.js | 41 ++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 5 deletions(-) diff --git a/tests/lib/rules/no-duplicate-imports.js b/tests/lib/rules/no-duplicate-imports.js index 42d35c85751..d3fe72d6490 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,8 @@ 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\";", { code: "import os from \"os\";\nexport { hello } from \"hello\";", options: [{ includeExports: true }] @@ -45,6 +47,14 @@ ruleTester.run("no-duplicate-imports", rule, { { code: "import { merge } from \"lodash-es\";\nexport { merge as lodashMerge }", 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 +63,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 +87,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 +101,17 @@ ruleTester.run("no-duplicate-imports", rule, { errors: [{ messageId: "exportAs", data: { module: "os" }, type: "ExportNamedDeclaration" }] }, { - code: "import os from \"os\";\nexport * from \"os\";", + 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" }] } From af199efa2687ce70f72e97eb21072122a2e86fd6 Mon Sep 17 00:00:00 2001 From: soufianeboutahlil Date: Sun, 30 May 2021 02:13:57 +0100 Subject: [PATCH 9/9] 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 | 15 +++++++++------ lib/rules/no-duplicate-imports.js | 20 ++++++++++---------- tests/lib/rules/no-duplicate-imports.js | 23 +++++++++++++++++++++++ 3 files changed, 42 insertions(+), 16 deletions(-) diff --git a/docs/rules/no-duplicate-imports.md b/docs/rules/no-duplicate-imports.md index ac536dd5922..437d2c3daa1 100644 --- a/docs/rules/no-duplicate-imports.md +++ b/docs/rules/no-duplicate-imports.md @@ -12,9 +12,7 @@ import { find } from 'module'; ## Rule Details -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. +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: @@ -38,7 +36,9 @@ import something from 'another-module'; Example of **correct** code for this rule: ```js -// not mergable, as they would require new nodes to be created. +/*eslint no-duplicate-imports: "error"*/ + +// not mergeable import { merge } from 'module'; import * as something from 'module'; ``` @@ -69,13 +69,16 @@ 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 +/*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 c1c1972bc4c..cc3da1d5a68 100644 --- a/lib/rules/no-duplicate-imports.js +++ b/lib/rules/no-duplicate-imports.js @@ -19,10 +19,10 @@ const NAMESPACE_TYPES = [ //------------------------------------------------------------------------------ /** - * Check if (import|export) type is belong to (ImportSpecifier|ExportSpecifier) or (ImportNamespaceSpecifier|ExportNamespaceSpecifier). - * @param {string} importExportType An import type to get. - * @param {string} type Specifier type can be namespace or specifier - * @returns {boolean} True if (import|export) type is belong to (ImportSpecifier|ExportSpecifier) or (ImportNamespaceSpecifier|ExportNamespaceSpecifier) and false if it doesn't. + * 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; @@ -58,8 +58,8 @@ function getImportExportType(node) { /** * Returns a boolean indicates if two (import|export) can be merged - * @param {ASTNode} node1 A node to get. - * @param {ASTNode} node2 A node to get. + * @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) { @@ -88,10 +88,10 @@ function isImportExportCanBeMerged(node1, node2) { } /** - * Returns a boolean if we should report (import/export). + * 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. + * @returns {boolean} True if the (import|export) should be reported. */ function shouldReportImportExport(node, previousNodes) { let i = 0; @@ -109,7 +109,7 @@ function shouldReportImportExport(node, previousNodes) { * 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, if the nodes are null we return []. + * @returns {[ASTNode]} An array contains only nodes with declarations types equal to type. */ function getNodesByDeclarationType(nodes, type) { return nodes @@ -130,7 +130,7 @@ function getModule(node) { } /** - * Checks if the (import|export) can be merged with at least one import and one export, and reports if so. + * 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. diff --git a/tests/lib/rules/no-duplicate-imports.js b/tests/lib/rules/no-duplicate-imports.js index d3fe72d6490..e200bbc704d 100644 --- a/tests/lib/rules/no-duplicate-imports.js +++ b/tests/lib/rules/no-duplicate-imports.js @@ -28,6 +28,7 @@ ruleTester.run("no-duplicate-imports", rule, { "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 }] @@ -48,6 +49,18 @@ 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 }] @@ -100,6 +113,16 @@ ruleTester.run("no-duplicate-imports", rule, { options: [{ includeExports: true }], errors: [{ messageId: "exportAs", data: { module: "os" }, type: "ExportNamedDeclaration" }] }, + { + 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 }],