Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recognize properly resolving @/*-aliased imports as internal #2334

Merged
merged 1 commit into from
Dec 24, 2021

Conversation

ombene
Copy link
Contributor

@ombene ombene commented Dec 20, 2021

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.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

CHANGELOG.md Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 21, 2021

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
Impacted file tree graph

@@           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
Copy link
Contributor Author

ombene commented Dec 21, 2021

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
Copy link
Member

ljharb commented Dec 21, 2021

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
Copy link
Contributor Author

ombene commented Dec 22, 2021

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

src/core/importType.js Outdated Show resolved Hide resolved
@ombene ombene force-pushed the internal-non-module-non-scoped branch from e60715e to 02c1dfb Compare December 22, 2021 22:35
src/core/importType.js Outdated Show resolved Hide resolved
src/core/importType.js Outdated Show resolved Hide resolved
src/core/importType.js Outdated Show resolved Hide resolved
@ljharb ljharb force-pushed the internal-non-module-non-scoped branch from 670b096 to ef980d4 Compare December 23, 2021 22:12
@ljharb ljharb merged commit ef980d4 into import-js:main Dec 24, 2021
@alumni
Copy link

alumni commented Jan 4, 2022

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Aliasing with @ is not recognized as 'internal'
3 participants