import-js / import-js/eslint-plugin-import
no-relative-parent-imports reports internal packages as parent
- 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
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