Repository navigation
Conversation
When an npm package lists the same name in both "dependencies" and "peerDependencies", the peer entry was dropped and only the prod entry kept. That made bun install a nested copy even when the root already provided a version satisfying the peer range. yarn and pnpm both treat the entry as a peer in this situation and dedupe to the consumer's copy. Package::from_npm now replaces the prod entry with the peer entry (same rotate-and-overwrite it already does for optionalDependencies duplicates), so the dependency participates in peer hoisting. Fixes #7869
|
Updated 4:09 AM PT - Aug 1st, 2026
✅ @robobun, your commit c7acd5dc73f5acdc9c89401b7b7d56b911d7a350 passed in 🧪 To try this PR locally: bunx bun-pr 36631That installs a local version of the PR into your bun-36631 --bun |
WalkthroughChangesPeer dependency deduplication
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/install/bun-install-registry.test.ts`:
- Around line 4032-4037: Remove the implementation-history explanatory comments
at test/cli/install/bun-install-registry.test.ts lines 4032-4037 and 4089-4090,
retaining only the existing issue URL comment at each regression test.
- Around line 4049-4073: Update both subprocess tests in
test/cli/install/bun-install-registry.test.ts at lines 4049-4073 and 4101-4114:
start stdout.text(), stderr.text(), and exited concurrently via one Promise.all;
in the second site, consume stdout as well as stderr and process exit. Preserve
the existing output and exit assertions after all three promises resolve.
- Around line 4087-4121: Update the fallback fixture and assertions in the test
“dependency listed in both dependencies and peerDependencies installs when root
does not provide it” so the peer dependency range resolves to a different
available version than the production dependency’s 2.0.0. Replace the broad
version setup with a distinct peer range and assert that the installed no-deps
package matches the peer-resolved version, proving peer resolution rather than
production resolution.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5e51551e-c686-4590-a0bc-670a152d8ec7
📒 Files selected for processing (2)
src/install/lockfile/Package.rstest/cli/install/bun-install-registry.test.ts
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/install/lockfile/Package.rs:836-837— This only patchesPackage::from_npm(registry manifests). The sibling pathPackage::parse_dependency(~line 1969, used viaparse_with_jsonfor git/tarball/folder/workspace packages) still pushes both the prod and peer entries for the same name —check_for_duplicate_dependenciesis false for those Features and the block is gated on!group.behavior.is_peer()— so a git/tarball package with the samedependencies+peerDependenciesshape can still nest. Per REVIEW.md ('fix the whole class… if a site is intentionally excluded, say so'), worth either mirroring the peer-over-prod override there or noting why that path is left as-is. Not blocking — pre-existing, and the registry path is the one #7869 hits.Extended reasoning...
What this is about
The PR converts a peer/prod name collision in
Package::from_npmfrom drop the peer, keep the prod entry to override the prod entry with the peer entry. That fixes #7869 for packages resolved from an npm registry manifest.There is a twin site that builds a
Package's dependency list from a rawpackage.jsonrather than a registry manifest:Package::parse_dependency, invoked viaparse_with_json. That path is used for git dependencies (processDependencyList.rs:159,Features::NPM), local/remote tarballs (processDependencyList.rs:237,256,Features::NPM),file:/folder deps (folder_resolver.rs:458,Features::FOLDER), workspace packages (Features::WORKSPACE), and cache-folder resolves (folder_resolver.rs:485). Its duplicate handling atPackage.rs:1969is:if FEATURES.check_for_duplicate_dependencies && !group.behavior.is_peer() && !group.behavior.is_workspace() { ... /* optional overrides; else warn */ }
Why the sibling still exhibits the symptom
For every non-root feature set (
NPM,FOLDER,WORKSPACE),check_for_duplicate_dependenciesisfalse(onlyFeatures::MAINsets it —resolver_hooks.rs:1250-1258), so this block is skipped entirely andparse_dependencyreturnsSome(this_dep)for both thedependenciesentry and thepeerDependenciesentry. The package ends up with two dependency records for the same name: one prod-behavior with the exact version ("2.0.0") and one peer-behavior with the range ("*").The prod record is a normal dependency — it is enqueued and resolved on its own version regardless of what the root provides. So installing
styled-componentsviagit+https://…or a tarball URL (instead of the registry) with a rootpostcss@^8should still produce the nestednode_modules/styled-components/node_modules/postcss@8.4.31this PR eliminates for the registry path.Addressing the counter-argument
One reading is that the two sites don't share a bug class:
from_npmdropped the peer,parse_dependencykeeps it, so "peer never participates in hoisting" isn't the pattern here. That's true of the mechanism, but the user-visible class being fixed is "same name in dependencies + peerDependencies causes an unnecessary nested copy when the consumer already satisfies the peer range". On theparse_dependencypath the peer participates, but the retained prod entry with an exact version is still a hard constraint the resolver must satisfy — the peer entry existing alongside it doesn't relax that. The PR's own new comment states the intent: "a package that lists the same name in both … is asking to defer to whatever version its consumer provides" — that intent isn't yet applied when the package arrives via git/tarball/folder/workspace.Step-by-step
- Root
package.json:{ "dependencies": { "no-deps": "1.0.0", "pkg": "git+https://…/pkg.git" } }. pkg'spackage.json:"dependencies": { "no-deps": "2.0.0" },"peerDependencies": { "no-deps": "*" }.- Resolver clones the git repo and calls
Package::parse_with_jsonwithFeatures::NPM(check_for_duplicate_dependencies = false). parse_dependencyruns for thedependenciesgroup → block at 1969 skipped (flag false) → pushes{no-deps, prod, "2.0.0"}.parse_dependencyruns for thepeerDependenciesgroup → block skipped (flag false, andis_peer()would exclude it anyway) → pushes{no-deps, peer, "*"}.- Both records land in
package_dependencies. The prod"2.0.0"record is enqueued as a normal dependency; root hasno-deps@1.0.0, so2.0.0nests underpkg/node_modules/— the exact tree #7869 reports, unchanged by this PR.
Suggested action
Either apply the equivalent peer-over-prod override in
parse_dependency(suppress/replace the earlier prod entry when a peer of the same name is being pushed, for thecheck_for_duplicate_dependencies = falsecallers), or add a line to the PR description stating that git/tarball/folder/workspace packages are intentionally out of scope for this change. The code shape there is different enough (no drop-and-continueto convert; would need to look back intopackage_dependenciesfor a same-name prod entry) that it's fine to defer to a follow-up — but per REVIEW.md the exclusion should be stated.Filing as a nit: nothing regresses, the npm-registry path is overwhelmingly the common case for #7869, and this is pre-existing behavior in code the PR does not touch.
- Root
-
🔴
src/install/lockfile/Package.rs:775-778— Unconditionally replacing the prod entry with a peer entry drops the "hard runtime dependency" signal, so in two untested scenarios the package ends up with nothing installed: (1) whenpeerDependenciesMetamarks the peer{optional: true}, the replacement getsPEER|OPTIONAL(lines 800-804) and is never enqueued (PackageManagerEnqueue.rs:640early-returns foris_optional_peer()); (2) when the user runs with--omit=peer/install.peer=false, the pure-PEERentry failsis_enabled(remote_package_features)atTree.rs:602-611and is filtered out. Before this PR the prod entry was kept and the dep was always installed; after,node_modulesis missing a package the author explicitly listed independencies. Consider gating the override on the peer being non-optional (i >= non_optional_peer_dependencies_start) and either preserving a prod/required bit on the replacement or skipping replacement so--omit=peerstill installs it.Extended reasoning...
What the bug is
The new prod→peer override at
Package.rs:775-776fires whenevergroup.behavior.is_peer() && dependency.behavior.is_prod(). It rotates out the existingdependenciesentry and writes a new entry whosebehavioris initialised fromgroup.behaviorat line 799 — i.e. purePEER, with noPRODbit retained. That discards the one signal that told the resolver "this package cannot function without this dependency". Two independent, user-reachable paths then drop the dependency entirely.Trigger 1 —
peerDependenciesMeta: { foo: { optional: true } }Immediately after the override decision, lines 800-804 set
Behavior::OPTIONALon the peer entry when(i as u32) < package_version.non_optional_peer_dependencies_start. Pernpm.rs, the first N entries of the peer list are the ones marked optional inpeerDependenciesMeta. So a package publishing:"dependencies": { "foo": "1.0.0" }, "peerDependencies": { "foo": "^1.0.0" }, "peerDependenciesMeta": { "foo": { "optional": true } }
now yields a single
PEER|OPTIONALentry instead of the previousPRODentry. Thenenqueue_dependency_with_main_and_success_fnatPackageManagerEnqueue.rs:640does:if dependency.behavior.is_optional_peer() { return Ok(()); }
so
foois never resolved or fetched.Tree.rs:559/815andisolated_install.rs:843treat unresolved optional peers as skippable. If the root does not providefoo, nothing installs it. This is exactly the "fallback" pattern (ship my own copy unless you give me one, and don't warn if you don't) that the linked issue is about — and it now regresses from "always installed" to "silently missing".Trigger 2 —
--omit=peer/[install] peer = falsefrom_npmis invoked with the constantFeatures::NPM(PackageManagerEnqueue.rs:2044), which hard-codespeer_dependencies = trueviabase()(resolver_hooks.rs:1242,1280-1283). So the peer group is always processed and the prod entry is always replaced, regardless of user options.But the tree-building filter uses
manager.options.remote_package_features(Tree.rs:602-611) and callsBehavior::is_enabled, which for a purePEERentry returnsfeatures.peer_dependencies && self.is_peer()(resolver_hooks.rs:261-267).remote_package_features.peer_dependenciesis set tofalseby--omit=peer(PackageManagerOptions.rs:741-744) orinstall.peer=falsein bunfig (PackageManagerOptions.rs:501-504).Using the PR's own styled-components example under
bun install --omit=peer:- Before this PR:
from_npmkept thePRODentry →is_enabled()→is_prod()→ true →postcss@8.4.31installed. - After this PR:
from_npmreplacesPRODwithPEER→is_enabled({peer_dependencies:false})→ false →postcssnot installed at all, even though styled-components declared it as a hard runtime dependency.
This trigger is distinct from trigger 1 — it applies to required peers as well, and a fix that only skips replacement for optional peers would not address it.
Why existing code doesn't prevent it
The override decision at line 776 checks
group.behavior, which is always plainPEER— theOPTIONALbit is added per-entry afterwards at lines 800-804, so the override cannot see it. And nothing on the write side (line 845) preserves the fact that the slot used to bePROD. Both new tests use thefallback-peer-depsfixture, which has nopeerDependenciesMetaand is run without--omit=peer, so neither path is exercised (REVIEW.md: "Cover the variant matrix — both states of every flag").Impact
Both are behavioral regressions from prior bun releases: a package that previously had its declared
dependenciesentry satisfied now ends up with the module missing fromnode_modules, producing a runtimeCannot find modulefor the consumer. The second trigger hits a documented CLI flag / bunfig option; the first hits any package using the optional-peer-with-fallback pattern.Step-by-step proof (trigger 1)
- Group order (
Package.rs:652-663):DEPENDENCIESruns first →foowritten withBehavior::PRODat indexj. PEERgroup runs. Duplicate scan findsfooatj;group.behavior.is_peer() && dependency.behavior.is_prod()→duplicate_at = Some(j).- Line 799:
behavior = group.behavior=PEER. Line 801-804:i < non_optional_peer_dependencies_start(foo is inpeerDependenciesMetaas optional) →behavior |= OPTIONAL. - Line 837-847: prod entry at
jrotated out; slot overwritten with thePEER|OPTIONALdependency. - Enqueue:
PackageManagerEnqueue.rs:640seesis_optional_peer()→return Ok(()). Nothing resolvesfoo. - Tree build: unresolved optional peer → skipped.
node_modules/fooabsent.
How to fix
Two options, non-exclusive:
- Only take the peer-over-prod branch when the peer is required: check
(i as u32) >= package_version.non_optional_peer_dependencies_startbefore settingduplicate_at(or clearBehavior::OPTIONALon the replacement when it overwrote a prod entry). - Preserve the prod signal on the replacement (e.g.
behavior |= Behavior::PRODwhenduplicate_atcame from a prod entry), sois_enabled()still returns true under--omit=peerand the dep is enqueued even without a root provider.
Add tests for both variants: a fixture with
peerDependenciesMeta.{name}.optional=true+ prod dep of the same name, and the existing fixture run under--omit=peer. - Before this PR:
When a name is listed in both dependencies and peerDependencies and the peer is marked optional in peerDependenciesMeta, the replacement must stay a required peer: the package listed it in dependencies, so it cannot be dropped when the root does not provide one. Skip the peerDependenciesMeta optional bit in that case. Also update the bun-add test that asserted the previous precedence (dependencies over peerDependencies) for transitive npm packages; that is the behaviour this change intentionally flips to match yarn/pnpm. The root package variant in bun-install.test.ts is unchanged.
|
Addressed the review feedback in c7acd5d:
|
There was a problem hiding this comment.
Both prior findings (optional-peer regression and the undrained stdout) are addressed in c7acd5d, and I didn't find anything new. Deferring to a human for the policy call: this inverts from_npm's dep-vs-peer precedence (and flips the existing bun-add test that asserted the old order), which is a resolution-semantics decision worth a maintainer sign-off even though it aligns with yarn/pnpm.
What was reviewed:
duplicate_at.is_none()gate at Package.rs:791 — confirmed a prod-shadowed optional peer stays required, covered by the newfallback-optional-peer-depsfixture/tests.- Rotate-and-overwrite reuse — same mechanism as the existing optional-over-prod path; peer only overrides when the earlier entry is
PROD, so optional/dev duplicates keep their prior handling. - Fallback test's
no-deps@2.0.0assertion — holds because the fixture registry'slatestis 2.0.0, so*resolves there.
Extended reasoning...
Overview
The PR changes Package::from_npm in src/install/lockfile/Package.rs so that when an npm package lists the same name in both dependencies and peerDependencies, the peer entry replaces the prod entry (previously the peer entry was dropped). It reuses the existing rotate-and-overwrite path that already handles optional-over-prod. A follow-up commit gates the OPTIONAL bit from peerDependenciesMeta on duplicate_at.is_none() so a prod-shadowed optional peer stays required. Tests: four new registry tests (dedupe-to-root and installs-when-root-absent for both required- and optional-peer fixtures), a new fallback-optional-peer-deps verdaccio fixture, and an inverted expectation in bun-add.test.ts ("should prefer dependencies over peerDependencies" → "should prefer peerDependencies over dependencies", 0.0.3 → 0.0.5).
Security risks
None identified. This is dependency-graph shaping inside the lockfile builder; no new inputs are parsed and no trust boundaries move.
Level of scrutiny
Medium-high. The Rust diff is ~10 lines, but it changes which package version ends up in node_modules for any transitive dep that appears in both groups — a resolution-semantics change that affects every install of packages shaped like styled-components 6.x. The PR flips an existing test to assert the opposite behaviour, which is a deliberate policy reversal rather than a mechanical fix. That's the kind of ecosystem-facing decision a maintainer should approve explicitly, especially since npm itself still keeps the prod entry (npm/cli#7106) and this chooses the yarn/pnpm behaviour instead.
Other factors
- My earlier 🔴 finding (optional-peer + prod → dep dropped entirely) was fixed correctly: line 791 now skips the
OPTIONALbit whenduplicate_atis set, and the newfallback-optional-peer-depsfixture covers both root-provides and root-absent variants. - The peer override is scoped to
dependency.behavior.is_prod(), so a name that already went through the optional-over-prod override (behavior = OPTIONAL) won't be re-overridden by peer; dev duplicates stillcontinue 'listas before. - The "installs when root does not provide it" test asserts
no-deps@2.0.0, which works because the peer range is*and the fixture registry'sdist-tags.latestis 2.0.0 — not because the prod version literal survives. That's fine as a guard but worth noting if theno-depsfixture ever gains a newer version. - All CodeRabbit / comment-cop threads are resolved and the current comment in Package.rs is a two-line precedence note, not a workaround justification.
|
Closing this PR because it has been inactive for more than 90 days. |
|
This pull request stays closed. Its description has errors, and its design is wrong. This comment corrects the record. Errors in the description
Error in the designThe change replaces the New measurementsEach run used a new directory that held only
pnpm 11.18.0, the version used in August, gave a different result. With no #7869 still reproduces on |
|
Two more measurements, and the reason this pull request stays closed. Two more rowsThe method is the same as in the table above. Each cell is the
Current rule on mainmain keeps the
A change to that rule needs a decision from a maintainer. A project can get one copy today with |
Related to #7869
What does this PR do?
When an npm package lists the same name in both
dependenciesandpeerDependencies, bun was dropping the peer entry and keeping only thedependenciesentry. That made bun install a nested copy even when the consumer already provided a version that satisfies the peer range.Package::from_npmnow replaces the prod entry with the peer entry for these duplicates (same rotate-and-overwrite it already does foroptionalDependenciesduplicates), so the dependency participates in peer hoisting and dedupes to the consumer's copy. The replacement is kept as a required peer even whenpeerDependenciesMetamarks it optional, because the package listed it independenciesand cannot work without it.Repro
{ "dependencies": { "postcss": "^8", "styled-components": "6.1.5-rc.0" } }styled-components@6.1.5-rc.0declarespostcssin both places:Package manager comparison
With the root package above (root provides
postcss@8.5.25):node_modules/postcssnode_modules/styled-components/node_modules/postcssnpm still has this bug (npm/cli#7106); yarn and pnpm both honour the peer entry, which is the behaviour this PR matches.
Scope and behaviour notes
Package::from_npm(packages resolved from an npm registry manifest). The siblingparse_with_jsonpath (git/tarball/folder/workspace package.json) is unchanged; it already pushes both a prod and a peer entry for the same name, and the registry path is the case peerDependencies not honored over dependencies when both are specified #7869 reports.dependencies.bun install --omit=peerthe replaced entry is now treated as a peer and omitted, matching pnpm. Previously thedependenciesentry survived--omit=peer.How did you verify your code works?
New tests in
test/cli/install/bun-install-registry.test.tsusing thefallback-peer-depsfixture (dependencies.no-deps: "2.0.0"+peerDependencies.no-deps: "*") and a newfallback-optional-peer-depsfixture (same shape pluspeerDependenciesMeta.no-deps.optional: true):dependency listed in both dependencies and peerDependencies dedupes to the root copy: fails on main (3 packages, nestedno-deps@2.0.0), passes with this change (2 packages, no nested copy)dependency listed in both dependencies and an optional peerDependency dedupes to the root copy: same for the optional-peer variant... installs when root does not provide it(both fixtures): guards that the package still gets its dependency when the root provides nothingAlso updated
test/cli/install/bun-add.test.tswhich previously asserted the opposite precedence for a transitive npm package. Ran thehoisting,peer,duplicate,bun-lockandisolated-installpeer suites; all green.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-add.test.ts test/cli/install/bun-install-registry.test.ts