import-js / import-js/eslint-plugin-import

no-relative-parent-imports reports internal packages as parent

Open
#2,467 3 comments 2 reactions 0 assignees View on GitHub
Dominant language
JavaScript
Stars
5.9k
Forks
1.5k
PR merge metrics
No merged PRs in 30d

Description

I recently started working on a rush monorepo, that basically has the setup:

```
rush.json
/lib
/lib/src/index.js -- export const abc = 123
/apps
/apps/src/index.js -- import { abc } from '@my-package/lib';
/apps/node_modules/@my-package/lib/src/index.js
```

I have eslint settings configured as:

```
settings: {
'import/internal-regex': '^@my-package/',
},
```

The rule `import/no-relative-parent-imports` wrongly reports that `import { abc } from '@my-package/lib'` with error ``Relative imports from parent directories are not allowed. Please either pass what you're importing through at runtime (dependency injection), move `index.ts` to same directory as `@my-package/lib` or consider making `@my-package/lib` a package.``

I suspect this is related to https://github.com/import-js/eslint-plugin-import/issues/1644

Upon some digging, it appears that this library _does_ correctly identify that import as "internal" (therefore rules like `import/order` work as expected) but then the path is "resolved" to `../node_modules/@my-package/lib/index.js` which looks like a parent directory.

[This check](https://github.com/import-js/eslint-plugin-import/blob/main/src/rules/no-relative-parent-imports.js#L24) returns false because it is an `internal` import, not `external`
[`absDepthPath`](https://github.com/import-js/eslint-plugin-import/blob/main/src/rules/no-relative-parent-imports.js#L28) resolves to the full node_modules path (e.g. `/user/jacob/my-repo/apps/node_modules/@my-package/lib/index.js`) then [`relDepthPath`](https://github.com/import-js/eslint-plugin-import/blob/main/src/rules/no-relative-parent-imports.js#L34) looks like a [parent import](https://github.com/import-js/eslint-plugin-import/blob/main/src/rules/no-relative-parent-imports.js#L36) when converted to a relative path (e.g. `../node_modules/@my-package/lib/index.js`)

My suggestion is to simply exit early on both `internal` and `builtin` (because why not?) modules instead of trying to resolve the path. Is there a reason it was not implemented that way in the first place? This library has generalized logic for detecting "parent" vs "sibling" logic already?

It is worth noting that I worked with this rule in an npm/lerna workspace and never came across this issue. So I tried investigating those and it appears in those workspaces, `node_modules` was hoisted to the top-most directory and it appears [`resolve`](https://github.com/import-js/eslint-plugin-import/blob/main/utils/resolve.js#L217) actually [failed to resolve](https://github.com/import-js/eslint-plugin-import/blob/main/src/rules/no-relative-parent-imports.js#L30) those and it would not throw an error.

So in that sense it seems that Rush is actually providing a _better_ node_modules resolution, and as a consequence these rules which might have allowed internal packages to slip by are now exposing a logical flaw...

Happy to open the PR to allow `internal` + `builtin` modules to exit early, but I suspect I am missing a piece of the logic, so opening this issue for discussion!

Contributor guide

Open the contributing guide

Research direction

Start with src/rules/no-relative-parent-imports.js and trace resolution through utils/resolve.js using the Rush monorepo example in the issue. Check how internal and builtin imports are classified before comparing resolved paths. Done means internal package imports are not reported as parent imports while genuine relative parent imports still are.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript
Domain
tooling
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.