From ef980d4b116cc5473990a633ef5214b6e37468b0 Mon Sep 17 00:00:00 2001 From: Omri Bernstein Date: Mon, 20 Dec 2021 17:15:43 -0500 Subject: [PATCH] [Fix] `importType`: properly resolve `@/*`-aliased imports as internal --- CHANGELOG.md | 3 + src/core/importType.js | 78 ++++++++++++------- src/rules/extensions.js | 1 - src/rules/no-cycle.js | 1 - tests/src/core/importType.js | 32 +++++--- tests/src/rules/no-extraneous-dependencies.js | 17 ++++ tests/src/rules/no-internal-modules.js | 24 ++++++ tests/src/rules/order.js | 39 +++++++++- 8 files changed, 154 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bac6b0040..88d7c411f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange ### Fixed - `importType`: avoid crashing on a non-string' ([#2305], thanks [@ljharb]) - [`first`]: prevent crash when parsing angular templates ([#2210], thanks [@ljharb]) +- `importType`: properly resolve `@/*`-aliased imports as internal ([#2334], thanks [@ombene]) ### Changed - [`no-default-import`]: report on the token "default" instead of the entire node ([#2299], thanks [@pmcelhaney]) @@ -950,6 +951,7 @@ for info on changes for earlier releases. [`memo-parser`]: ./memo-parser/README.md +[#2334]: https://github.com/import-js/eslint-plugin-import/pull/2334 [#2305]: https://github.com/import-js/eslint-plugin-import/pull/2305 [#2299]: https://github.com/import-js/eslint-plugin-import/pull/2299 [#2297]: https://github.com/import-js/eslint-plugin-import/pull/2297 @@ -1571,6 +1573,7 @@ for info on changes for earlier releases. [@noelebrun]: https://github.com/noelebrun [@ntdb]: https://github.com/ntdb [@nwalters512]: https://github.com/nwalters512 +[@ombene]: https://github.com/ombene [@ota-meshi]: https://github.com/ota-meshi [@panrafal]: https://github.com/panrafal [@paztis]: https://github.com/paztis diff --git a/src/core/importType.js b/src/core/importType.js index 3e579b944..ebdb306bc 100644 --- a/src/core/importType.js +++ b/src/core/importType.js @@ -13,6 +13,11 @@ function baseModule(name) { return pkg; } +function isInternalRegexMatch(name, settings) { + const internalScope = (settings && settings['import/internal-regex']); + return internalScope && new RegExp(internalScope).test(name); +} + export function isAbsolute(name) { return typeof name === 'string' && nodeIsAbsolute(name); } @@ -25,33 +30,18 @@ export function isBuiltIn(name, settings, path) { return isCoreModule(base) || extras.indexOf(base) > -1; } -export function isExternalModule(name, settings, path, context) { - if (arguments.length < 4) { - throw new TypeError('isExternalModule: name, settings, path, and context are all required'); +export function isExternalModule(name, path, context) { + if (arguments.length < 3) { + throw new TypeError('isExternalModule: name, path, and context are all required'); } - return (isModule(name) || isScoped(name)) && isExternalPath(name, settings, path, getContextPackagePath(context)); -} - -export function isExternalModuleMain(name, settings, path, context) { - return isModuleMain(name) && isExternalPath(name, settings, path, getContextPackagePath(context)); + return (isModule(name) || isScoped(name)) && typeTest(name, context, path) === 'external'; } -function isExternalPath(name, settings, path, packagePath) { - const internalScope = (settings && settings['import/internal-regex']); - if (internalScope && new RegExp(internalScope).test(name)) { - return false; - } - - if (!path || relative(packagePath, path).startsWith('..')) { - return true; +export function isExternalModuleMain(name, path, context) { + if (arguments.length < 3) { + throw new TypeError('isExternalModule: name, path, and context are all required'); } - - 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('..'); - }); + return isModuleMain(name) && typeTest(name, context, path) === 'external'; } const moduleRegExp = /^\w/; @@ -87,17 +77,49 @@ function isRelativeToSibling(name) { return /^\.[\\/]/.test(name); } -function typeTest(name, context, path) { +function isExternalPath(path, context) { + if (!path) { + return false; + } + + const { settings } = context; + const packagePath = getContextPackagePath(context); + + if (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('..'); + }); +} + +function isInternalPath(path, context) { + if (!path) { + return false; + } + const packagePath = getContextPackagePath(context); + return !relative(packagePath, path).startsWith('../'); +} + +function isExternalLookingName(name) { + return isModule(name) || isScoped(name); +} + +function typeTest(name, context, path ) { const { settings } = context; + if (isInternalRegexMatch(name, settings)) { return 'internal'; } if (isAbsolute(name, settings, path)) { return 'absolute'; } if (isBuiltIn(name, settings, path)) { return 'builtin'; } - 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'; } + if (isExternalPath(path, context)) { return 'external'; } + if (isInternalPath(path, context)) { return 'internal'; } + if (isExternalLookingName(name)) { return 'external'; } return 'unknown'; } diff --git a/src/rules/extensions.js b/src/rules/extensions.js index 677a0a095..8596cbfd0 100644 --- a/src/rules/extensions.js +++ b/src/rules/extensions.js @@ -159,7 +159,6 @@ module.exports = { // determine if this is a module const isPackage = isExternalModule( importPath, - context.settings, resolve(importPath, context), context, ) || isScoped(importPath); diff --git a/src/rules/no-cycle.js b/src/rules/no-cycle.js index cefa41f99..e61c3be26 100644 --- a/src/rules/no-cycle.js +++ b/src/rules/no-cycle.js @@ -44,7 +44,6 @@ module.exports = { const maxDepth = typeof options.maxDepth === 'number' ? options.maxDepth : Infinity; const ignoreModule = (name) => options.ignoreExternal && isExternalModule( name, - context.settings, resolve(name, context), context, ); diff --git a/tests/src/core/importType.js b/tests/src/core/importType.js index 4f4a862a6..937f19303 100644 --- a/tests/src/core/importType.js +++ b/tests/src/core/importType.js @@ -75,6 +75,17 @@ describe('importType(name)', function () { expect(importType('constants', pathContext)).to.equal('internal'); }); + it("should return 'internal' for aliased internal modules that are found, even if they are not discernible as scoped", function () { + // `@` for internal modules is a common alias and is different from scoped names. + // Scoped names are prepended with `@` (e.g. `@scoped/some-file.js`) whereas `@` + // as an alias by itelf is the full root name (e.g. `@/some-file.js`). + const alias = { '@': path.join(pathToTestFiles, 'internal-modules') }; + const webpackConfig = { resolve: { alias } }; + const pathContext = testContext({ 'import/resolver': { webpack: { config: webpackConfig } } }); + expect(importType('@/api/service', pathContext)).to.equal('internal'); + expect(importType('@/does-not-exist', pathContext)).to.equal('unknown'); + }); + it("should return 'parent' for internal modules that go through the parent", function () { expect(importType('../foo', context)).to.equal('parent'); expect(importType('../../foo', context)).to.equal('parent'); @@ -100,6 +111,7 @@ describe('importType(name)', function () { it("should return 'unknown' for any unhandled cases", function () { expect(importType(' /malformed', context)).to.equal('unknown'); expect(importType(' foo', context)).to.equal('unknown'); + expect(importType('-/no-such-path', context)).to.equal('unknown'); }); it("should return 'builtin' for additional core modules", function () { @@ -233,20 +245,20 @@ describe('importType(name)', function () { it('`isExternalModule` works with windows directory separator', function () { const context = testContext(); - expect(isExternalModule('foo', {}, 'E:\\path\\to\\node_modules\\foo', context)).to.equal(true); - expect(isExternalModule('@foo/bar', {}, 'E:\\path\\to\\node_modules\\@foo\\bar', context)).to.equal(true); - expect(isExternalModule('foo', { - 'import/external-module-folders': ['E:\\path\\to\\node_modules'], - }, 'E:\\path\\to\\node_modules\\foo', context)).to.equal(true); + expect(isExternalModule('foo', 'E:\\path\\to\\node_modules\\foo', context)).to.equal(true); + expect(isExternalModule('@foo/bar', 'E:\\path\\to\\node_modules\\@foo\\bar', context)).to.equal(true); + expect(isExternalModule('foo', 'E:\\path\\to\\node_modules\\foo', testContext({ + settings: { 'import/external-module-folders': ['E:\\path\\to\\node_modules'] }, + }))).to.equal(true); }); it('`isExternalModule` works with unix directory separator', function () { const context = testContext(); - expect(isExternalModule('foo', {}, '/path/to/node_modules/foo', context)).to.equal(true); - expect(isExternalModule('@foo/bar', {}, '/path/to/node_modules/@foo/bar', context)).to.equal(true); - expect(isExternalModule('foo', { - 'import/external-module-folders': ['/path/to/node_modules'], - }, '/path/to/node_modules/foo', context)).to.equal(true); + expect(isExternalModule('foo', '/path/to/node_modules/foo', context)).to.equal(true); + expect(isExternalModule('@foo/bar', '/path/to/node_modules/@foo/bar', context)).to.equal(true); + expect(isExternalModule('foo', '/path/to/node_modules/foo', testContext({ + settings: { 'import/external-module-folders': ['/path/to/node_modules'] }, + }))).to.equal(true); }); it('correctly identifies scoped modules with `isScoped`', () => { diff --git a/tests/src/rules/no-extraneous-dependencies.js b/tests/src/rules/no-extraneous-dependencies.js index 131604ad9..d4c9f6d7f 100644 --- a/tests/src/rules/no-extraneous-dependencies.js +++ b/tests/src/rules/no-extraneous-dependencies.js @@ -164,6 +164,23 @@ ruleTester.run('no-extraneous-dependencies', rule, { `, settings: { 'import/resolver': 'webpack' }, }), + + test({ + code: 'import "@custom-internal-alias/api/service";', + settings: { + 'import/resolver': { + webpack: { + config: { + resolve: { + alias: { + '@custom-internal-alias': testFilePath('internal-modules'), + }, + }, + }, + }, + }, + }, + }), ], invalid: [ test({ diff --git a/tests/src/rules/no-internal-modules.js b/tests/src/rules/no-internal-modules.js index 2fee9f450..4a733d142 100644 --- a/tests/src/rules/no-internal-modules.js +++ b/tests/src/rules/no-internal-modules.js @@ -304,6 +304,30 @@ ruleTester.run('no-internal-modules', rule, { column: 8, } ], }), + test({ + code: 'import "@/api/service";', + options: [ { + forbid: [ '**/api/*' ], + } ], + errors: [ { + message: 'Reaching to "@/api/service" is not allowed.', + line: 1, + column: 8, + } ], + settings: { + 'import/resolver': { + webpack: { + config: { + resolve: { + alias: { + '@': testFilePath('internal-modules'), + }, + }, + }, + }, + }, + }, + }), // exports test({ code: 'export * from "./plugin2/index.js";\nexport * from "./plugin2/app/index"', diff --git a/tests/src/rules/order.js b/tests/src/rules/order.js index 4b15cec89..79426c4c4 100644 --- a/tests/src/rules/order.js +++ b/tests/src/rules/order.js @@ -1,4 +1,4 @@ -import { test, getTSParsers, getNonDefaultParsers } from '../utils'; +import { test, getTSParsers, getNonDefaultParsers, testFilePath } from '../utils'; import { RuleTester } from 'eslint'; import eslintPkg from 'eslint/package.json'; @@ -847,6 +847,43 @@ ruleTester.run('order', rule, { ], }), ]), + // Using `@/*` to alias internal modules + test({ + code: ` + import fs from 'fs'; + + import express from 'express'; + + import service from '@/api/service'; + + import fooParent from '../foo'; + + import fooSibling from './foo'; + + import index from './'; + + import internalDoesNotExistSoIsUnknown from '@/does-not-exist'; + `, + options: [ + { + groups: ['builtin', 'external', 'internal', 'parent', 'sibling', 'index', 'unknown'], + 'newlines-between': 'always', + }, + ], + settings: { + 'import/resolver': { + webpack: { + config: { + resolve: { + alias: { + '@': testFilePath('internal-modules'), + }, + }, + }, + }, + }, + }, + }), ], invalid: [ // builtin before external module (require)