Repository: import-js/eslint-plugin-import (js)

Issue:
Aliasing with `@` is not recognized as 'internal'
This seems to be a repeat of an issue that has been resolved and regressed twice now, see https://github.com/import-js/eslint-plugin-import/issues/2145 and https://github.com/import-js/eslint-plugin-import/issues/2249.

In my case the source code is private, but we're using a config that looks like:

```jsonc
// ...
"import/resolver": {
  "alias": [
    ["@", "./src"]
  ]
}
// ...
```

(See also https://github.com/johvin/eslint-import-resolver-alias.) And with source code like:

```jsx
// ...
import exampleInternalViaAlias from '@/exampleInternalViaAlias';

import exampleSibling from './exampleSibling';
// ...
```

When upgrading to v2.25.3 from v2.22.1, without any other changes to source code or other dependencies, we have now started getting false negatives like:

```
  3:1  error  `@/exampleInternalViaAlias` import should occur after import of `./exampleSibling`  import/order
```

Injecting some logs into the installed version of `eslint-plugin-import` in our node_modules, I can see that the import type is getting calculated as `'unknown'` (instead of `'internal'`) for `'@/exampleInternalViaAlias'`.

Pull request:
Recognize properly resolving `@/*`-aliased imports as internal
Aims to close #2333.

The issue this PR addresses seems to have cropped up twice before. I've tried to include a new test case that will hopefully help prevent regressions. Also, I've tried to make the various "type" cases more independent, out of the belief that this will be more robust and future-proof. But I'd love any feedback! This is my first contribution, so I'm a little wary that I might be changing too many things at once, without the proper context for the decisions that led the code to be in its present state.

Looking at the code, it seemed like a good idea to have the check for "is this internal" stop being dependent on either "is this scoped" or "is this a module", and also to separate the "is this internal" logic from the "is this external" logic.

In doing so, it seemed prudent to put the internal path check after the external path check, so that `node_modules` (or any other external path that happens to be in the root project directory) would correctly calculate as `'external'`. In doing that, it became apparent that the [`import/internal-regex`](https://github.com/import-js/eslint-plugin-import/blob/main/README.md#importinternal-regex) configuration should take precedence, so I made that its own function and I actually decided to put it before everything—presumably the regex should take precedence over even types that would otherwise resolve as "absolute" or "built-in", say.

One final complication is `'external'` can also include import names that look like (potentially scoped) modules, even if the names don't resolve to any particular path, but only as long as the names don't resolve to an internal path. So I decided to split that "external-looking name check" logic out into its own function, which could be called separately after the the internal check. This also meant augmenting other `isExternalPath` consumers to call this new function as well.

Some other context is that I'm curious whether / why this logic around "external-looking names" is necessary. The architecture as a whole seems a lot more straightforward if `eslint-plugin-import` relies on its resolvers to provide paths, and treats any path misses as `'unknown'`—but perhaps I'm missing some important use case, like maybe web-URL-based imports? So by putting that logic into its own function, I hoped to: (a) enrich the discussion around whether / why we need this (by giving it a concrete name / location in the code), (b) make it more straightforward to delete if indeed that seems wise, or possibly better rename it to whatever use cases it serves.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index bac6b004..88d7c411 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 3e579b94..ebdb306b 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 677a0a09..8596cbfd 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 cefa41f9..e61c3be2 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,
     );


Test patch:
diff --git a/tests/src/core/importType.js b/tests/src/core/importType.js
index 4f4a862a..937f1930 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 131604ad..d4c9f6d7 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 2fee9f45..4a733d14 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 4b15cec8..79426c4c 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)