Skip to content

Commit

Permalink
[Fix] importType: properly resolve @/*-aliased imports as internal
Browse files Browse the repository at this point in the history
  • Loading branch information
ombene authored and ljharb committed Dec 20, 2021
1 parent e156316 commit ef980d4
Show file tree
Hide file tree
Showing 8 changed files with 154 additions and 41 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
78 changes: 50 additions & 28 deletions src/core/importType.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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/;
Expand Down Expand Up @@ -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';
}

Expand Down
1 change: 0 additions & 1 deletion src/rules/extensions.js
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,6 @@ module.exports = {
// determine if this is a module
const isPackage = isExternalModule(
importPath,
context.settings,
resolve(importPath, context),
context,
) || isScoped(importPath);
Expand Down
1 change: 0 additions & 1 deletion src/rules/no-cycle.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
Expand Down
32 changes: 22 additions & 10 deletions tests/src/core/importType.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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 () {
Expand Down Expand Up @@ -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`', () => {
Expand Down
17 changes: 17 additions & 0 deletions tests/src/rules/no-extraneous-dependencies.js
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down
24 changes: 24 additions & 0 deletions tests/src/rules/no-internal-modules.js
Original file line number Diff line number Diff line change
Expand Up @@ -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"',
Expand Down
39 changes: 38 additions & 1 deletion tests/src/rules/order.js
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit ef980d4

Please sign in to comment.