From 802ce7d49d912289d590f735bf9bd2d931064863 Mon Sep 17 00:00:00 2001 From: JEROMEH Date: Tue, 24 Mar 2020 09:03:01 +0100 Subject: [PATCH] [Fix] `extensions`/`no-cycle`/`no-extraneous-dependencies`: Correct module real path resolution add real support of isAbsolute (windows + unix support) importType refactoring: use the real resolved package path to check if external of internal, and not the name only like before: in case of monorepo, external modules are not under node_modules due to symlink but still out of the module. correct tests node_modules dependencies to really provide teh package.json like in real usage correct no-extraneous-dependencies rule: get the real name from the resolved package.json. If not aliased imports (alias/react for example) will not be correctly interpreted change path import add real support of isAbsolute (windows + unix support) correct no-extraneous-dependencies rule: get the real name from the resolved package.json. If not aliased imports (alias/react for example) will not be correctly interpreted even externals like "a/file.js" must not use extension. only module names like 'module.js' and '@scope/module.js' are allowed correct bad external definition: must be the folder path under the root of the module. Here the module root is test folder, not cycles folder --- CHANGELOG.md | 1 + package.json | 1 + src/core/importType.js | 72 ++++++++++--------- src/core/packagePath.js | 18 +++++ src/rules/extensions.js | 5 +- src/rules/no-cycle.js | 3 +- src/rules/no-extraneous-dependencies.js | 19 +++-- .../node_modules/@generated/bar/package.json | 3 + .../node_modules/@generated/foo/package.json | 3 + .../@org/not-a-dependency/package.json | 3 + .../node_modules/@org/package/package.json | 3 + tests/files/node_modules/a/package.json | 3 + tests/files/node_modules/chai/package.json | 3 + .../node_modules/es6-module/package.json | 3 + .../eslint-import-resolver-foo/package.json | 3 + tests/files/node_modules/exceljs/package.json | 1 + tests/files/node_modules/jquery/package.json | 3 + .../node_modules/jsx-module/package.json | 3 + tests/files/node_modules/left-pad | 1 - tests/files/node_modules/left-pad/index.js | 0 .../node_modules/left-pad/not-a-dependency | 0 .../files/node_modules/left-pad/package.json | 3 + .../not-a-dependency/package.json | 3 + tests/files/node_modules/react | 1 - tests/files/node_modules/react/index.js | 1 + .../files/node_modules/react/not-a-dependency | 0 tests/files/node_modules/react/package.json | 3 + tests/files/webpack.config.js | 3 + tests/src/core/importType.js | 15 ++-- tests/src/rules/no-extraneous-dependencies.js | 8 +++ tests/src/rules/order.js | 19 +---- 31 files changed, 140 insertions(+), 67 deletions(-) create mode 100644 src/core/packagePath.js create mode 100644 tests/files/node_modules/@generated/bar/package.json create mode 100644 tests/files/node_modules/@generated/foo/package.json create mode 100644 tests/files/node_modules/@org/not-a-dependency/package.json create mode 100644 tests/files/node_modules/@org/package/package.json create mode 100644 tests/files/node_modules/a/package.json create mode 100644 tests/files/node_modules/chai/package.json create mode 100644 tests/files/node_modules/es6-module/package.json create mode 100644 tests/files/node_modules/eslint-import-resolver-foo/package.json create mode 100644 tests/files/node_modules/jquery/package.json create mode 100644 tests/files/node_modules/jsx-module/package.json delete mode 120000 tests/files/node_modules/left-pad create mode 100644 tests/files/node_modules/left-pad/index.js create mode 100644 tests/files/node_modules/left-pad/not-a-dependency create mode 100644 tests/files/node_modules/left-pad/package.json create mode 100644 tests/files/node_modules/not-a-dependency/package.json delete mode 120000 tests/files/node_modules/react create mode 100644 tests/files/node_modules/react/index.js create mode 100644 tests/files/node_modules/react/not-a-dependency create mode 100644 tests/files/node_modules/react/package.json diff --git a/CHANGELOG.md b/CHANGELOG.md index 54f9886cf..bd503f04a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel - [`no-cycle`]: fix perf regression ([#1944], thanks [@Blasz]) - [`first`]: fix handling of `import = require` ([#1963], thanks [@MatthiasKunnen]) - [`no-cycle`]/[`extensions`]: fix isExternalModule usage ([#1696], thanks [@paztis]) +- [`extensions`]/[`no-cycle`]/[`no-extraneous-dependencies`]: Correct module real path resolution ([#1696], thanks [@paztis]) ### Changed - [Generic Import Callback] Make callback for all imports once in rules ([#1237], thanks [@ljqx]) diff --git a/package.json b/package.json index f584a8dda..eb259e4aa 100644 --- a/package.json +++ b/package.json @@ -104,6 +104,7 @@ "doctrine": "1.5.0", "eslint-import-resolver-node": "^0.3.4", "eslint-module-utils": "^2.6.0", + "find-up": "^2.0.0", "has": "^1.0.3", "is-core-module": "^1.0.2", "minimatch": "^3.0.4", diff --git a/src/core/importType.js b/src/core/importType.js index 82a47dd05..ecea976f4 100644 --- a/src/core/importType.js +++ b/src/core/importType.js @@ -1,6 +1,8 @@ +import { isAbsolute as nodeIsAbsolute, relative, resolve as nodeResolve } from 'path'; import isCoreModule from 'is-core-module'; import resolve from 'eslint-module-utils/resolve'; +import { getContextPackagePath } from './packagePath'; function baseModule(name) { if (isScoped(name)) { @@ -12,7 +14,7 @@ function baseModule(name) { } export function isAbsolute(name) { - return name && name.startsWith('/'); + return nodeIsAbsolute(name); } // path is defined only when a resolver resolves to a non-standard path @@ -23,35 +25,43 @@ export function isBuiltIn(name, settings, path) { return isCoreModule(base) || extras.indexOf(base) > -1; } -function isExternalPath(path, name, settings) { - const folders = (settings && settings['import/external-module-folders']) || ['node_modules']; - return !path || folders.some(folder => isSubpath(folder, path)); +export function isExternalModule(name, settings, path, context) { + if (arguments.length < 4) { + throw new TypeError('isExternalModule: name, settings, path, and context are all required'); + } + return isModule(name) && isExternalPath(name, settings, path, getContextPackagePath(context)); +} + +export function isExternalModuleMain(name, settings, path, context) { + return isModuleMain(name) && isExternalPath(name, settings, path, getContextPackagePath(context)); } -function isSubpath(subpath, path) { - const normPath = path.replace(/\\/g, '/'); - const normSubpath = subpath.replace(/\\/g, '/').replace(/\/$/, ''); - if (normSubpath.length === 0) { +function isExternalPath(name, settings, path, packagePath) { + const internalScope = (settings && settings['import/internal-regex']); + if (internalScope && new RegExp(internalScope).test(name)) { return false; } - const left = normPath.indexOf(normSubpath); - const right = left + normSubpath.length; - return left !== -1 && - (left === 0 || normSubpath[0] !== '/' && normPath[left - 1] === '/') && - (right >= normPath.length || normPath[right] === '/'); -} -const externalModuleRegExp = /^(?:\w|@)/; -export function isExternalModule(name, settings, path) { - if (arguments.length < 3) { - throw new TypeError('isExternalModule: name, settings, and path are all required'); + if (!path || relative(packagePath, path).startsWith('..')) { + return true; } - return externalModuleRegExp.test(name) && isExternalPath(path, name, settings); + + const folders = (settings && settings['import/external-module-folders']) || ['node_modules']; + return folders.some((folder) => { + const folderPath = nodeResolve(packagePath, folder); + const relativePath = relative(folderPath, path); + return !relativePath.startsWith('..'); + }); +} + +const moduleRegExp = /^\w/; +function isModule(name) { + return name && moduleRegExp.test(name); } -const externalModuleMainRegExp = /^[\w]((?!\/).)*$/; -export function isExternalModuleMain(name, settings, path) { - return externalModuleMainRegExp.test(name) && isExternalPath(path, name, settings); +const moduleMainRegExp = /^[\w]((?!\/).)*$/; +function isModuleMain(name) { + return name && moduleMainRegExp.test(name); } const scopedRegExp = /^@[^/]*\/?[^/]+/; @@ -64,12 +74,6 @@ export function isScopedMain(name) { return name && scopedMainRegExp.test(name); } -function isInternalModule(name, settings, path) { - const internalScope = (settings && settings['import/internal-regex']); - const matchesScopedOrExternalRegExp = scopedRegExp.test(name) || externalModuleRegExp.test(name); - return (matchesScopedOrExternalRegExp && (internalScope && new RegExp(internalScope).test(name) || !isExternalPath(path, name, settings))); -} - function isRelativeToParent(name) { return/^\.\.$|^\.\.[\\/]/.test(name); } @@ -83,12 +87,14 @@ function isRelativeToSibling(name) { return /^\.[\\/]/.test(name); } -function typeTest(name, settings, path) { +function typeTest(name, context, path) { + const { settings } = context; if (isAbsolute(name, settings, path)) { return 'absolute'; } if (isBuiltIn(name, settings, path)) { return 'builtin'; } - if (isInternalModule(name, settings, path)) { return 'internal'; } - if (isExternalModule(name, settings, path)) { return 'external'; } - if (isScoped(name, settings, path)) { return 'external'; } + if (isModule(name, settings, path) || isScoped(name, settings, path)) { + const packagePath = getContextPackagePath(context); + return isExternalPath(name, settings, path, packagePath) ? 'external' : 'internal'; + } if (isRelativeToParent(name, settings, path)) { return 'parent'; } if (isIndex(name, settings, path)) { return 'index'; } if (isRelativeToSibling(name, settings, path)) { return 'sibling'; } @@ -100,5 +106,5 @@ export function isScopedModule(name) { } export default function resolveImportType(name, context) { - return typeTest(name, context.settings, resolve(name, context)); + return typeTest(name, context, resolve(name, context)); } diff --git a/src/core/packagePath.js b/src/core/packagePath.js new file mode 100644 index 000000000..e95b06666 --- /dev/null +++ b/src/core/packagePath.js @@ -0,0 +1,18 @@ +import { dirname } from 'path'; +import findUp from 'find-up'; +import readPkgUp from 'read-pkg-up'; + + +export function getContextPackagePath(context) { + return getFilePackagePath(context.getFilename()); +} + +export function getFilePackagePath(filePath) { + const fp = findUp.sync('package.json', { cwd: filePath }); + return dirname(fp); +} + +export function getFilePackageName(filePath) { + const { pkg } = readPkgUp.sync({ cwd: filePath, normalize: false }); + return pkg && pkg.name; +} diff --git a/src/rules/extensions.js b/src/rules/extensions.js index 1b2475ec5..bd47afa99 100644 --- a/src/rules/extensions.js +++ b/src/rules/extensions.js @@ -130,8 +130,8 @@ module.exports = { function isExternalRootModule(file) { const slashCount = file.split('/').length - 1; + if (slashCount === 0) return true; if (isScopedModule(file) && slashCount <= 1) return true; - if (isExternalModule(file, context, resolve(file, context)) && !slashCount) return true; return false; } @@ -160,7 +160,8 @@ module.exports = { const isPackage = isExternalModule( importPath, context.settings, - resolve(importPath, context) + resolve(importPath, context), + context ) || isScoped(importPath); if (!extension || !importPath.endsWith(`.${extension}`)) { diff --git a/src/rules/no-cycle.js b/src/rules/no-cycle.js index 77a24fefb..74b77cbc3 100644 --- a/src/rules/no-cycle.js +++ b/src/rules/no-cycle.js @@ -45,7 +45,8 @@ module.exports = { const ignoreModule = (name) => options.ignoreExternal && isExternalModule( name, context.settings, - resolve(name, context) + resolve(name, context), + context ); function checkSourceValue(sourceNode, importer) { diff --git a/src/rules/no-extraneous-dependencies.js b/src/rules/no-extraneous-dependencies.js index e2b0eadab..2e541584f 100644 --- a/src/rules/no-extraneous-dependencies.js +++ b/src/rules/no-extraneous-dependencies.js @@ -5,6 +5,7 @@ import minimatch from 'minimatch'; import resolve from 'eslint-module-utils/resolve'; import moduleVisitor from 'eslint-module-utils/moduleVisitor'; import importType from '../core/importType'; +import { getFilePackageName } from '../core/packagePath'; import docsUrl from '../docsUrl'; const depFieldCache = new Map(); @@ -116,6 +117,15 @@ function optDepErrorMessage(packageName) { `not optionalDependencies.`; } +function getModuleOriginalName(name) { + const [first, second] = name.split('/'); + return first.startsWith('@') ? `${first}/${second}` : first; +} + +function getModuleRealName(resolved) { + return getFilePackageName(resolved); +} + function reportIfMissing(context, deps, depsOptions, node, name) { // Do not report when importing types if (node.importKind === 'type' || (node.parent && node.parent.importKind === 'type')) { @@ -129,10 +139,11 @@ function reportIfMissing(context, deps, depsOptions, node, name) { const resolved = resolve(name, context); if (!resolved) { return; } - const splitName = name.split('/'); - const packageName = splitName[0][0] === '@' - ? splitName.slice(0, 2).join('/') - : splitName[0]; + // get the real name from the resolved package.json + // if not aliased imports (alias/react for example) will not be correctly interpreted + // fallback on original name in case no package.json found + const packageName = getModuleRealName(resolved) || getModuleOriginalName(name); + const isInDeps = deps.dependencies[packageName] !== undefined; const isInDevDeps = deps.devDependencies[packageName] !== undefined; const isInOptDeps = deps.optionalDependencies[packageName] !== undefined; diff --git a/tests/files/node_modules/@generated/bar/package.json b/tests/files/node_modules/@generated/bar/package.json new file mode 100644 index 000000000..b70db688d --- /dev/null +++ b/tests/files/node_modules/@generated/bar/package.json @@ -0,0 +1,3 @@ +{ + "name": "@generated/bar" +} diff --git a/tests/files/node_modules/@generated/foo/package.json b/tests/files/node_modules/@generated/foo/package.json new file mode 100644 index 000000000..c5d0d6b33 --- /dev/null +++ b/tests/files/node_modules/@generated/foo/package.json @@ -0,0 +1,3 @@ +{ + "name": "@generated/foo" +} diff --git a/tests/files/node_modules/@org/not-a-dependency/package.json b/tests/files/node_modules/@org/not-a-dependency/package.json new file mode 100644 index 000000000..a81c5f291 --- /dev/null +++ b/tests/files/node_modules/@org/not-a-dependency/package.json @@ -0,0 +1,3 @@ +{ + "name": "@org/not-a-dependency" +} diff --git a/tests/files/node_modules/@org/package/package.json b/tests/files/node_modules/@org/package/package.json new file mode 100644 index 000000000..7cb5d73da --- /dev/null +++ b/tests/files/node_modules/@org/package/package.json @@ -0,0 +1,3 @@ +{ + "name": "@org/package" +} diff --git a/tests/files/node_modules/a/package.json b/tests/files/node_modules/a/package.json new file mode 100644 index 000000000..44d21f1fa --- /dev/null +++ b/tests/files/node_modules/a/package.json @@ -0,0 +1,3 @@ +{ + "name": "a" +} diff --git a/tests/files/node_modules/chai/package.json b/tests/files/node_modules/chai/package.json new file mode 100644 index 000000000..00acdd2ca --- /dev/null +++ b/tests/files/node_modules/chai/package.json @@ -0,0 +1,3 @@ +{ + "name": "chai" +} diff --git a/tests/files/node_modules/es6-module/package.json b/tests/files/node_modules/es6-module/package.json new file mode 100644 index 000000000..0bff4dda0 --- /dev/null +++ b/tests/files/node_modules/es6-module/package.json @@ -0,0 +1,3 @@ +{ + "name": "es6-module" +} diff --git a/tests/files/node_modules/eslint-import-resolver-foo/package.json b/tests/files/node_modules/eslint-import-resolver-foo/package.json new file mode 100644 index 000000000..190e8e6e4 --- /dev/null +++ b/tests/files/node_modules/eslint-import-resolver-foo/package.json @@ -0,0 +1,3 @@ +{ + "name": "eslint-import-resolver-foo" +} diff --git a/tests/files/node_modules/exceljs/package.json b/tests/files/node_modules/exceljs/package.json index 70d59eaaa..f2412292d 100644 --- a/tests/files/node_modules/exceljs/package.json +++ b/tests/files/node_modules/exceljs/package.json @@ -1,3 +1,4 @@ { + "name": "exceljs", "main": "./excel.js" } diff --git a/tests/files/node_modules/jquery/package.json b/tests/files/node_modules/jquery/package.json new file mode 100644 index 000000000..e0563fbf4 --- /dev/null +++ b/tests/files/node_modules/jquery/package.json @@ -0,0 +1,3 @@ +{ + "name": "jquery" +} diff --git a/tests/files/node_modules/jsx-module/package.json b/tests/files/node_modules/jsx-module/package.json new file mode 100644 index 000000000..6edbe5fc9 --- /dev/null +++ b/tests/files/node_modules/jsx-module/package.json @@ -0,0 +1,3 @@ +{ + "name": "jsx-module" +} diff --git a/tests/files/node_modules/left-pad b/tests/files/node_modules/left-pad deleted file mode 120000 index dbbbe75d2..000000000 --- a/tests/files/node_modules/left-pad +++ /dev/null @@ -1 +0,0 @@ -not-a-dependency \ No newline at end of file diff --git a/tests/files/node_modules/left-pad/index.js b/tests/files/node_modules/left-pad/index.js new file mode 100644 index 000000000..e69de29bb diff --git a/tests/files/node_modules/left-pad/not-a-dependency b/tests/files/node_modules/left-pad/not-a-dependency new file mode 100644 index 000000000..e69de29bb diff --git a/tests/files/node_modules/left-pad/package.json b/tests/files/node_modules/left-pad/package.json new file mode 100644 index 000000000..a95a5e067 --- /dev/null +++ b/tests/files/node_modules/left-pad/package.json @@ -0,0 +1,3 @@ +{ + "name": "left-pad" +} diff --git a/tests/files/node_modules/not-a-dependency/package.json b/tests/files/node_modules/not-a-dependency/package.json new file mode 100644 index 000000000..857233121 --- /dev/null +++ b/tests/files/node_modules/not-a-dependency/package.json @@ -0,0 +1,3 @@ +{ + "name": "not-a-dependency" +} diff --git a/tests/files/node_modules/react b/tests/files/node_modules/react deleted file mode 120000 index dbbbe75d2..000000000 --- a/tests/files/node_modules/react +++ /dev/null @@ -1 +0,0 @@ -not-a-dependency \ No newline at end of file diff --git a/tests/files/node_modules/react/index.js b/tests/files/node_modules/react/index.js new file mode 100644 index 000000000..8b1378917 --- /dev/null +++ b/tests/files/node_modules/react/index.js @@ -0,0 +1 @@ + diff --git a/tests/files/node_modules/react/not-a-dependency b/tests/files/node_modules/react/not-a-dependency new file mode 100644 index 000000000..e69de29bb diff --git a/tests/files/node_modules/react/package.json b/tests/files/node_modules/react/package.json new file mode 100644 index 000000000..bcbea4166 --- /dev/null +++ b/tests/files/node_modules/react/package.json @@ -0,0 +1,3 @@ +{ + "name": "react" +} diff --git a/tests/files/webpack.config.js b/tests/files/webpack.config.js index 980c32425..6a5dc0b88 100644 --- a/tests/files/webpack.config.js +++ b/tests/files/webpack.config.js @@ -2,5 +2,8 @@ module.exports = { resolve: { extensions: ['', '.js', '.jsx'], root: __dirname, + alias: { + 'alias/chai$': 'chai' // alias for no-extraneous-dependencies tests + } }, } diff --git a/tests/src/core/importType.js b/tests/src/core/importType.js index 5eb655e55..371a3d739 100644 --- a/tests/src/core/importType.js +++ b/tests/src/core/importType.js @@ -128,7 +128,7 @@ describe('importType(name)', function () { it("should return 'internal' for module from 'node_modules' if 'node_modules' missed in 'external-module-folders'", function() { const foldersContext = testContext({ 'import/external-module-folders': [] }); - expect(importType('resolve', foldersContext)).to.equal('internal'); + expect(importType('chai', foldersContext)).to.equal('internal'); }); it("should return 'internal' for module from 'node_modules' if its name matched 'internal-regex'", function() { @@ -188,7 +188,7 @@ describe('importType(name)', function () { const foldersContext = testContext({ 'import/resolver': 'webpack', - 'import/external-module-folders': ['files/symlinked-module'], + 'import/external-module-folders': ['symlinked-module'], }); expect(importType('@test-scope/some-module', foldersContext)).to.equal('external'); }); @@ -202,7 +202,7 @@ describe('importType(name)', function () { const foldersContext_2 = testContext({ 'import/resolver': 'webpack', - 'import/external-module-folders': ['les/symlinked-module'], + 'import/external-module-folders': ['ymlinked-module'], }); expect(importType('@test-scope/some-module', foldersContext_2)).to.equal('internal'); }); @@ -210,7 +210,7 @@ describe('importType(name)', function () { it('returns "external" for a scoped module from a symlinked directory which partial path ending w/ slash is contained in "external-module-folders" (webpack resolver)', function() { const foldersContext = testContext({ 'import/resolver': 'webpack', - 'import/external-module-folders': ['files/symlinked-module/'], + 'import/external-module-folders': ['symlinked-module/'], }); expect(importType('@test-scope/some-module', foldersContext)).to.equal('external'); }); @@ -218,7 +218,7 @@ describe('importType(name)', function () { it('returns "internal" for a scoped module from a symlinked directory when "external-module-folders" contains an absolute path resembling directory‘s relative path (webpack resolver)', function() { const foldersContext = testContext({ 'import/resolver': 'webpack', - 'import/external-module-folders': ['/files/symlinked-module'], + 'import/external-module-folders': ['/symlinked-module'], }); expect(importType('@test-scope/some-module', foldersContext)).to.equal('internal'); }); @@ -232,10 +232,11 @@ describe('importType(name)', function () { }); it('`isExternalModule` works with windows directory separator', function() { - expect(isExternalModule('foo', {}, 'E:\\path\\to\\node_modules\\foo')).to.equal(true); + const context = testContext(); + expect(isExternalModule('foo', {}, 'E:\\path\\to\\node_modules\\foo', context)).to.equal(true); expect(isExternalModule('foo', { 'import/external-module-folders': ['E:\\path\\to\\node_modules'], - }, 'E:\\path\\to\\node_modules\\foo')).to.equal(true); + }, 'E:\\path\\to\\node_modules\\foo', context)).to.equal(true); }); it('correctly identifies scoped modules with `isScopedModule`', () => { diff --git a/tests/src/rules/no-extraneous-dependencies.js b/tests/src/rules/no-extraneous-dependencies.js index 56eb555eb..c94e9f977 100644 --- a/tests/src/rules/no-extraneous-dependencies.js +++ b/tests/src/rules/no-extraneous-dependencies.js @@ -314,6 +314,14 @@ ruleTester.run('no-extraneous-dependencies', rule, { message: '\'not-a-dependency\' should be listed in the project\'s dependencies. Run \'npm i -S not-a-dependency\' to add it', }], }), + test({ + code: 'import chai from "alias/chai";', + settings: { 'import/resolver': 'webpack' }, + errors: [{ + // missing dependency is chai not alias + message: "'chai' should be listed in the project's dependencies. Run 'npm i -S chai' to add it", + }], + }), ], }); diff --git a/tests/src/rules/order.js b/tests/src/rules/order.js index b95681b42..c599a73c8 100644 --- a/tests/src/rules/order.js +++ b/tests/src/rules/order.js @@ -376,23 +376,6 @@ ruleTester.run('order', rule, { 'import/external-module-folders': ['node_modules', 'symlinked-module'], }, }), - // Monorepo setup, using Webpack resolver, partial workspace folder path - // in external-module-folders - test({ - code: ` - import _ from 'lodash'; - import m from '@test-scope/some-module'; - - import bar from './bar'; - `, - options: [{ - 'newlines-between': 'always', - }], - settings: { - 'import/resolver': 'webpack', - 'import/external-module-folders': ['node_modules', 'files/symlinked-module'], - }, - }), // Monorepo setup, using Node resolver (doesn't resolve symlinks) test({ code: ` @@ -406,7 +389,7 @@ ruleTester.run('order', rule, { }], settings: { 'import/resolver': 'node', - 'import/external-module-folders': ['node_modules', 'files/symlinked-module'], + 'import/external-module-folders': ['node_modules', 'symlinked-module'], }, }), // Option: newlines-between: 'always'