[Fix] extensions/no-cycle/no-extraneous-dependencies: Correct m…

…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

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])

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",

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

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;
+}

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}`)) {

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) {

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;

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
+    }
   },
 }

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,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');
   });
@@ -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`', () => {

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",
+      }],
+    }),
   ],
 });