fix(no-extraneous-dependencies): merge deps from closest package.json#482
fix(no-extraneous-dependencies): merge deps from closest package.json#482B4nan wants to merge 2 commits into
Conversation
In monorepos (lerna, npm workspaces, pnpm workspaces), `packageDir` is typically set to point at the repo root so that the shared devDeps are discoverable. The current behavior is "packageDir XOR closest" — setting packageDir makes the rule ignore the workspace package's own `package.json` entirely. That means every dep declared in the workspace package (lodash, internal `@scope/*` workspace packages, etc.) gets reported as "should be listed in the project's dependencies" even though it literally is. The fix: always seed `packageContent` from the closest `package.json` walking up from `context.physicalFilename`, then layer any packageDir entries on top. If packageDir is not set, only the closest is used (same as before). If it is set, the closest wins the base layer and any extra deps declared in the root still get merged in. This mirrors the long-standing downstream patch that `apify/apify-core` has been carrying on `eslint-plugin-import`, which we've been forced to re-apply on `eslint-plugin-import-x` since the migration. Upstreaming so we can drop the patch.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/rules/no-extraneous-dependencies.ts`:
- Around line 98-101: The current merge uses Object.assign(packageContent[key],
closestPackageContent[key]) which corrupts array fields like bundledDependencies
by treating arrays as objects; update the merge to handle arrays and objects
separately: in the loop over Object.keys(packageContent) (variables
packageContent, closestPackageContent, and type PackageDeps), check if both
values are arrays via Array.isArray and, if so, set packageContent[key] =
Array.from(new Set([...closestPackageContent[key], ...packageContent[key]])) (or
concatenate in the correct precedence) instead of Object.assign; otherwise, for
plain objects keep using Object.assign or deep-merge as needed. Apply the same
array-aware fix to the similar packageDir merge loop that handles
packageDir/closestPackageContent merging.
- Around line 119-122: The loop merging packageContent and packageContent_ uses
Object.assign which overwrites array fields (notably bundledDependencies)
instead of merging them; update the loop that iterates
depsKey/packageContent[key] to detect when both packageContent[key] and
packageContent_[key] are arrays (e.g., key === 'bundledDependencies') and
concatenate them deduplicating entries (e.g., via a Set), otherwise use
Object.assign for plain objects or simple assignment for primitives so arrays
are merged rather than replaced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60baff08-d3dc-4fbe-9ca7-30e43ac4aae5
📒 Files selected for processing (1)
src/rules/no-extraneous-dependencies.ts
| for (const depsKey of Object.keys(packageContent)) { | ||
| const key = depsKey as keyof PackageDeps | ||
| Object.assign(packageContent[key], closestPackageContent[key]) | ||
| } |
There was a problem hiding this comment.
Object.assign corrupts bundledDependencies array during merge.
bundledDependencies is an array, but Object.assign on arrays overwrites by numeric index rather than concatenating. For example:
Object.assign(['dep1', 'dep2'], ['dep3']) // → ['dep3', 'dep2']This loses 'dep1' and will cause false positives when the closest package.json declares bundled deps that get overwritten by a shorter array from packageDir.
🐛 Proposed fix: Handle array and object merging separately
if (closestPackageContent) {
for (const depsKey of Object.keys(packageContent)) {
const key = depsKey as keyof PackageDeps
- Object.assign(packageContent[key], closestPackageContent[key])
+ if (key === 'bundledDependencies') {
+ packageContent[key] = [
+ ...new Set([...packageContent[key], ...closestPackageContent[key]]),
+ ]
+ } else {
+ Object.assign(packageContent[key], closestPackageContent[key])
+ }
}
}Apply the same pattern at lines 119-121 for the packageDir merge loop.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/rules/no-extraneous-dependencies.ts` around lines 98 - 101, The current
merge uses Object.assign(packageContent[key], closestPackageContent[key]) which
corrupts array fields like bundledDependencies by treating arrays as objects;
update the merge to handle arrays and objects separately: in the loop over
Object.keys(packageContent) (variables packageContent, closestPackageContent,
and type PackageDeps), check if both values are arrays via Array.isArray and, if
so, set packageContent[key] = Array.from(new Set([...closestPackageContent[key],
...packageContent[key]])) (or concatenate in the correct precedence) instead of
Object.assign; otherwise, for plain objects keep using Object.assign or
deep-merge as needed. Apply the same array-aware fix to the similar packageDir
merge loop that handles packageDir/closestPackageContent merging.
| for (const depsKey of Object.keys(packageContent)) { | ||
| const key = depsKey as keyof PackageDeps | ||
| Object.assign(packageContent[key], packageContent_[key]) | ||
| } |
There was a problem hiding this comment.
Same bundledDependencies array merge issue here.
The same Object.assign problem applies in this loop when merging deps from packageDir entries.
🐛 Proposed fix
if (packageContent_) {
for (const depsKey of Object.keys(packageContent)) {
const key = depsKey as keyof PackageDeps
- Object.assign(packageContent[key], packageContent_[key])
+ if (key === 'bundledDependencies') {
+ packageContent[key] = [
+ ...new Set([...packageContent[key], ...packageContent_[key]]),
+ ]
+ } else {
+ Object.assign(packageContent[key], packageContent_[key])
+ }
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/rules/no-extraneous-dependencies.ts` around lines 119 - 122, The loop
merging packageContent and packageContent_ uses Object.assign which overwrites
array fields (notably bundledDependencies) instead of merging them; update the
loop that iterates depsKey/packageContent[key] to detect when both
packageContent[key] and packageContent_[key] are arrays (e.g., key ===
'bundledDependencies') and concatenate them deduplicating entries (e.g., via a
Set), otherwise use Object.assign for plain objects or simple assignment for
primitives so arrays are merged rather than replaced.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
TIP This summary will be updated as you push new changes. Give us feedback
commit: |
SukkaW
left a comment
SukkaW
left a comment
There was a problem hiding this comment.
I will take a look at this when I have some time. In the meantime, can you add tests so that your changes are verifiable?
…on merge behavior Move the react-from-nested-package test from invalid to valid since the rule now correctly finds react via the closest package.json. Add new valid cases covering: deps merged from both closest and packageDir entries, and unchanged behavior when closest IS the packageDir. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Problem
In a monorepo (lerna / npm workspaces / pnpm workspaces), the typical
eslint.config.jsfor the root setspackageDirto a list of paths that point at the repo root so shared devDeps are discoverable:```js
'import-x/no-extraneous-dependencies': ['error', {
packageDir: ['.', '../..', '../../..'],
}]
```
With the current rule, setting `packageDir` makes the rule ignore the closest `package.json` entirely. That's a problem: each workspace package has its own `package.json` with its own dependencies (that's the whole point of a workspace), and those deps become invisible to the rule. Every dep declared in the workspace package — `lodash`, internal `@apify-packages/*` workspace packages, etc. — gets reported as "should be listed in the project's dependencies" even though it literally is.
Concretely, on apify/apify-core (~60 lerna workspace packages), the migration from `eslint-plugin-import` to `eslint-plugin-import-x` surfaced hundreds of false positives like this:
```
src/packages/action-queue/src/action_queue.ts
1:1 error 'lodash' should be listed in the project's dependencies. Run 'npm i lodash' to add it import-x/no-extraneous-dependencies
3:1 error '@apify/log' should be listed in the project's dependencies. Run 'npm i @apify/log' to add it import-x/no-extraneous-dependencies
5:1 error '@apify-packages/aws' should be listed in the project's dependencies. Run 'npm i @apify-packages/aws' to add it import-x/no-extraneous-dependencies
```
...even though `src/packages/action-queue/package.json` declares all three.
`apify-core` has been carrying a local `patch-package` patch on `eslint-plugin-import` to work around this for years. We re-applied the same patch on `eslint-plugin-import-x` during the migration (see apify/apify-core#26946). This PR upstreams that fix so we can drop the patch.
Fix
Always seed `packageContent` from the closest `package.json` walking up from `context.physicalFilename`, then layer any `packageDir` entries on top:
The order is deliberate: entries later in `packageDir` can still override the closest match if there's a version conflict, matching the existing behavior where later `packageDir` entries already override earlier ones.
Verification
After this PR + re-applying it as a `patch-package` patch against `eslint-plugin-import-x` 4.16.2 locally: `npx eslint ./src/packages` on apify-core completes cleanly. Without it: hundreds of false-positive "should be listed in the project's dependencies" errors.
Companion PR
This is the second of two fixes I've sent to address the CI breakage caused by this issue. The first one (#481) was about native memory leaking in `legacyNodeResolve`; you suggested migrating to `import-x/resolver-next` which we've done via apify/apify-eslint-config#42. This one is a different bug — it's in the rule's deps-lookup, not the resolver — so the `resolver-next` migration doesn't cover it.
Happy to add a test; let me know what shape you'd prefer.
Summary by CodeRabbit