Recognize properly resolving @/*-aliased imports as internal

[ombene]

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 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.

[ljharb]

This seems like a good fix, but it makes me very nervous to be changing this much of the core importType logic.

Could we add some test cases to affected rules as well?

[codecov]

Codecov Report

Merging #2334 (670b096) into main (e156316) will not change coverage.
The diff coverage is n/a.

❗ Current head 670b096 differs from pull request most recent head ef980d4. Consider uploading reports for the commit ef980d4 to get more accurate results

@@           Coverage Diff           @@
##             main    #2334   +/-   ##
=======================================
  Coverage   94.64%   94.64%           
=======================================
  Files          65       65           
  Lines        2691     2691           
  Branches      891      891           
=======================================
  Hits         2547     2547           
  Misses        144      144           

Impacted Files	Coverage Δ
src/core/importType.js	100.00% <ø> (ø)
src/rules/extensions.js	100.00% <ø> (ø)
src/rules/no-cycle.js	97.67% <ø> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e156316...ef980d4. Read the comment docs.

[ombene]

Could we add some test cases to affected rules as well?

Yeah sounds like a great idea! In terms of new test cases, I can imagine something sensible for import/order and import/no-internal-modules. Does that sound right?

[ljharb]

That sounds great. There's a lot of rules that use importType, but those along with no-extraneous-dependencies might be the only ones that care about whether something is "internal" or not.

[ombene]

👍 I took a crack at it. What do you think?

[alumni]

Cool! This also solved the tsconfig path alias "@modules/*": ["src/modules/*"] being identified as external in one out of two similar projects inside a monorepo. I will try to figure out what's going on in the remaining project, but this refactor is a great improvement!