Correct module real path resolution

CHANGELOG.md

@@ -15,6 +15,8 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel
 - [`no-unused-modules`]: make type imports mark a module as used (fixes #1924) ([#1974], thanks [@cherryblossom000])
 - [`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])
@@ -788,6 +790,7 @@ for info on changes for earlier releases.
 [#1719]: https://github.com/benmosher/eslint-plugin-import/pull/1719
 [#1704]: https://github.com/benmosher/eslint-plugin-import/issues/1704
 [#1702]: https://github.com/benmosher/eslint-plugin-import/issues/1702
+[#1696]: https://github.com/benmosher/eslint-plugin-import/pull/1696
 [#1691]: https://github.com/benmosher/eslint-plugin-import/pull/1691
 [#1690]: https://github.com/benmosher/eslint-plugin-import/pull/1690
 [#1689]: https://github.com/benmosher/eslint-plugin-import/pull/1689
@@ -1310,3 +1313,4 @@ for info on changes for earlier releases.
 [@swernerx]: https://github.com/swernerx
 [@fsmaia]: https://github.com/fsmaia
 [@MatthiasKunnen]: https://github.com/MatthiasKunnen
+[@paztis]: https://github.com/paztis

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,32 +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] === '/');
+
+  if (!path || relative(packagePath, path).startsWith('..')) {
+    return true;
+  }
+
+  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 externalModuleRegExp = /^\w/;
-export function isExternalModule(name, settings, path) {
-  return externalModuleRegExp.test(name) && isExternalPath(path, name, settings);
+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 = /^@[^/]*\/?[^/]+/;
@@ -61,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);
 }
@@ -80,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'; }
@@ -97,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;
     }
 
@@ -157,8 +157,12 @@ module.exports = {
       const extension = path.extname(resolvedPath || importPath).substring(1);
 
       // determine if this is a module
-      const isPackage = isExternalModule(importPath, context.settings)
-        || isScoped(importPath);
+      const isPackage = isExternalModule(
+        importPath,
+        context.settings,
+        resolve(importPath, context),
+        context
+      ) || isScoped(importPath);
 
       if (!extension || !importPath.endsWith(`.${extension}`)) {
         const extensionRequired = isUseOfExtensionRequired(extension, isPackage);

src/rules/no-cycle.js

@@ -3,6 +3,7 @@
  * @author Ben Mosher
  */
 
+import resolve from 'eslint-module-utils/resolve';
 import Exports from '../ExportMap';
 import { isExternalModule } from '../core/importType';
 import moduleVisitor, { makeOptionsSchema } from 'eslint-module-utils/moduleVisitor';
@@ -41,7 +42,12 @@ module.exports = {
 
     const options = context.options[0] || {};
     const maxDepth = typeof options.maxDepth === 'number' ? options.maxDepth : Infinity;
-    const ignoreModule = (name) => options.ignoreExternal ? isExternalModule(name) : false;
+    const ignoreModule = (name) => options.ignoreExternal && isExternalModule(
+      name,
+      context.settings,
+      resolve(name, context),
+      context
+    );
 
     function checkSourceValue(sourceNode, importer) {
       if (ignoreModule(sourceNode.value)) {

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