Skip to content

Commit

Permalink
[Fix] extensions/no-cycle/no-extraneous-dependencies: Correct m…
Browse files Browse the repository at this point in the history
…odule 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
  • Loading branch information
JEROMEH authored and ljharb committed Mar 24, 2020
1 parent e22fc53 commit 802ce7d
Show file tree
Hide file tree
Showing 31 changed files with 140 additions and 67 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
72 changes: 39 additions & 33 deletions src/core/importType.js
Original file line number Diff line number Diff line change
@@ -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)) {
Expand All @@ -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
Expand All @@ -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 = /^@[^/]*\/?[^/]+/;
Expand All @@ -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);
}
Expand All @@ -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'; }
Expand All @@ -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));
}
18 changes: 18 additions & 0 deletions src/core/packagePath.js
Original file line number Diff line number Diff line change
@@ -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;
}
5 changes: 3 additions & 2 deletions src/rules/extensions.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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}`)) {
Expand Down
3 changes: 2 additions & 1 deletion src/rules/no-cycle.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
19 changes: 15 additions & 4 deletions src/rules/no-extraneous-dependencies.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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')) {
Expand All @@ -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;
Expand Down
3 changes: 3 additions & 0 deletions tests/files/node_modules/@generated/bar/package.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions tests/files/node_modules/@generated/foo/package.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions tests/files/node_modules/@org/not-a-dependency/package.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions tests/files/node_modules/@org/package/package.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions tests/files/node_modules/a/package.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions tests/files/node_modules/chai/package.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions tests/files/node_modules/es6-module/package.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions tests/files/node_modules/exceljs/package.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions tests/files/node_modules/jquery/package.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions tests/files/node_modules/jsx-module/package.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion tests/files/node_modules/left-pad

This file was deleted.

Empty file.
Empty file.
3 changes: 3 additions & 0 deletions tests/files/node_modules/left-pad/package.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions tests/files/node_modules/not-a-dependency/package.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion tests/files/node_modules/react

This file was deleted.

1 change: 1 addition & 0 deletions tests/files/node_modules/react/index.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Empty file.
3 changes: 3 additions & 0 deletions tests/files/node_modules/react/package.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions tests/files/webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,8 @@ module.exports = {
resolve: {
extensions: ['', '.js', '.jsx'],
root: __dirname,
alias: {
'alias/chai$': 'chai' // alias for no-extraneous-dependencies tests
}
},
}
15 changes: 8 additions & 7 deletions tests/src/core/importType.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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');
});
Expand All @@ -202,23 +202,23 @@ 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');
});

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');
});

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');
});
Expand All @@ -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`', () => {
Expand Down
8 changes: 8 additions & 0 deletions tests/src/rules/no-extraneous-dependencies.js
Original file line number Diff line number Diff line change
Expand Up @@ -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",
}],
}),
],
});

Expand Down
Loading

0 comments on commit 802ce7d

Please sign in to comment.