Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
WalkthroughWorkspace dependency sorting now uses workspace paths as an equal-order tiebreaker before package names. CLI regression tests cover conflicting dependency versions, repeated reinstalls, and reversed workspace path layouts. ChangesWorkspace hoisting order
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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 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 3928-3931: Remove the explanatory comment at
test/cli/install/bun-install-registry.test.ts lines 3928-3931, the procedural
comment at lines 3999-3999, and the explanatory comment at lines 4019-4020;
retain only the regression test’s issue URL comment.
- Around line 3986-4013: Update both install test sites in
test/cli/install/bun-install-registry.test.ts (3986-4013 and 4052-4062) to
collect stderr.text() and exited concurrently with Promise.all before making
assertions. Apply the combined-result pattern to both installs at the anchor
site and the additional site, while preserving the existing stderr and
successful-exit assertions.
🪄 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: f8e5a19b-e78a-41ef-b994-a9fa8a0c0a30
📒 Files selected for processing (2)
src/install/lockfile.rstest/cli/install/bun-install-registry.test.ts
|
Updated 5:51 PM PT - Aug 12th, 2026
❌ @robobun, your commit d11647e has 2 failures in
🧪 To try this PR locally: bunx bun-pr 35571That installs a local version of the PR into your bun-35571 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline comment nit: checked that the new branch is scoped to behavior.is_workspace() && version.tag == Workspace on both sides, so non-workspace deps and the name fallback are unaffected; DepSorter is also used by the isolated linker (isolated_install.rs:664), where reordering workspace members among themselves is harmless. This does change which version wins the root slot for existing monorepos whose layout depended on the old name-order tie-break, so it's worth a human look.
Extended reasoning...
The Rust change is small and correctly guarded, and the tests exercise both orderings plus lockfile re-install. But this is a behavioral change to the hoisted linker's tie-break — any monorepo whose current install relied on name-order hoisting will get a different root node_modules layout after this lands. That's an intentional npm-compat correction, not a bug in the PR, but it's the kind of user-visible install-layout change a maintainer should sign off on rather than a bot.
There was a problem hiding this comment.
Thanks — the earlier feedback is addressed (comment trimmed to the npm-compat invariant, spawn output drained via Promise.all, test comments reduced to the issue URL). The remaining comment-cop flag on lockfile.rs:232 is a false positive on the 2-line comment; no further trimming needed there.
I'm deferring to a human on the merge itself: this changes bun install's hoisting tie-break for workspace members, which will alter on-disk node_modules layouts for existing monorepos where workspace name order ≠ path order — worth a maintainer sign-off on the npm-parity claim and lockfile stability.
What was reviewed
DepSorter::is_less_than: guard checksbehavior.is_workspace()and bothversion.tag == Workspacebefore callingversion.workspace(), so the union accessor is safe; falls through to name compare on equal paths.- Tests: fixture
strict-peer-depexists in the local registry; both path-order directions covered plus reinstall-from-lockfile;stdout: "ignore"avoids the pipe-buffer deadlock concern. - Checked that the branch only affects the workspace-vs-workspace tie-break (behavior.cmp already groups workspaces first), so non-workspace dep ordering is unchanged.
Extended reasoning...
Overview
The PR changes DepSorter::is_less_than in src/install/lockfile.rs to compare workspace entries by their relative path (version.workspace()) before falling back to package name, so the path-alphabetically-first workspace's direct deps win the root node_modules slot under the hoisted linker. Two new tests in test/cli/install/bun-install-registry.test.ts cover both orderings (path-first wants newer / path-first wants older), reinstall from a saved lockfile, install from a workspace subdirectory, and assert a peer dependent gets no nested copy.
Security risks
None. This is a deterministic comparator change over data already parsed from workspace package.json files; no new I/O, parsing, or trust boundary.
Level of scrutiny
Medium-high. The diff is small (14 lines of Rust) and mechanically sound — the Tag::Workspace guard makes the version.workspace() accessor safe, and Behavior::cmp already groups workspaces so the new branch only fires within that group. But it's a behavioral change to the package manager's hoisting algorithm that will change which version lands at root node_modules for existing monorepos whenever workspace name order and path order disagree. That's the kind of change where someone who owns src/install/ should confirm the npm-parity claim and check for knock-on effects on lockfile serialization order or the isolated linker.
Other factors
- My prior inline nit (bug-history in the code comment) was addressed in commit 82e0f48; the comment is now the durable invariant only. The subsequent comment-cop re-fire (hash
8b93ba2b14fb) is the automated multi-line detector tripping on a 2-line comment — not actionable. - CodeRabbit's concurrent-drain and test-comment nits were also addressed in the same commit. Its remaining unresolved thread ("regression-test comments") is stale against the current diff, which contains only the issue-URL comment.
- Tests use existing local-registry fixtures (
no-deps,strict-peer-dep), spawn withstdout: "ignore"+stderr: "pipe"and drain viaPromise.all, and follow the surroundinghoistingdescribe's conventions. - CI build
#80287was queued for the previous commit; no result posted for the head commit yet.
There was a problem hiding this comment.
No bugs found, but this changes bun install's hoisting tie-break for every workspace monorepo with conflicting dep versions — existing projects will get a different node_modules layout — so it warrants a human sign-off from someone who owns install. One thing worth confirming: DepSorter::is_less_than is also used by the isolated linker (src/install/isolated_install.rs:664), not just the hoisted tree builder, so the ordering change applies there too; the PR description and tests only cover --linker=hoisted.
The earlier feedback (trim the code comment to the invariant, drain spawn output concurrently, drop test narration) has all been addressed; the remaining comment-cop flag at lockfile.rs:232 is stale — it fired before 82e0f48 shortened the comment to two lines.
Extended reasoning...
Overview
The PR adds a 14-line branch to DepSorter::is_less_than in src/install/lockfile.rs so that when two dependencies both have workspace behavior and Tag::Workspace versions, they are compared by version.workspace() (the workspace's relative path) before falling back to the existing name comparison. Two new tests in the hoisting describe of bun-install-registry.test.ts exercise both orderings (path-first workspace wants the newer / older version), re-install from a saved lockfile, install from a workspace subdirectory, and assert a peer dependent gets no nested copy.
Security risks
None. This is a comparator tweak over lockfile-owned string data; no untrusted input parsing, allocation sizing, or filesystem path construction is added.
Level of scrutiny
High. DepSorter decides which version of a conflicting dependency is hoisted to the root node_modules, so this changes the on-disk layout for any monorepo where two workspaces depend on different versions of the same package. That's the intended fix for #9838, but it's also a silent behavior change for projects that currently happen to work with the name-order layout. DepSorter is shared between the hoisted tree builder (lockfile/Tree.rs:725) and the isolated linker (isolated_install.rs:664); the PR narrative and tests cover only the hoisted path. The npm-compat claim ("npm visits workspaces in path order") is plausible and matches the repro, but should be confirmed by a maintainer who knows Arborist's actual ordering — if npm's rule is subtler (e.g. workspaces array order rather than lexicographic path order), the reversed test would still pass here while diverging from npm.
Other factors
All prior review feedback (mine on the code-comment content, CodeRabbit's on test comments and concurrent pipe draining, the first comment-cop) has been addressed in 82e0f48. The unresolved comment-cop inline at line 232 predates that trim and no longer applies to the current two-line comment. Tests look well-constructed (both orderings, lockfile re-install, peer-dep nesting assertion, concurrent stderr/exit collection). Given the user-visible install-layout change and the untested isolated-linker call site, this should get a human review rather than a bot approval.
|
On the two points from review: Isolated linker: npm ordering: checked empirically that npm uses lexicographic path order, not the
|
There was a problem hiding this comment.
No issues found, but this changes hoisted-linker layout for any workspace monorepo where package-name order and path order disagree — worth a human sign-off on the compat trade-off.
What was reviewed:
Behavior::cmpreturnsEqualonly when both sides share the workspace bit, so the new branch never compares a workspace against a non-workspace.workspace_pathslookup isif let (Some, Some)— falls back to name order if either entry is missing, so no panic and no ordering change on unpopulated paths.- Isolated linker's
DepSorteruse atisolated_install.rs:664orders a queue, not a hoist target; author ran that suite green. - Tests cover both orderings, reinstall from lockfile,
--frozen-lockfile, and a peer dependent not getting a nested copy.
Extended reasoning...
Overview
The PR changes DepSorter::is_less_than in src/install/lockfile.rs so that when two dependencies both carry the WORKSPACE behavior bit (which Behavior::cmp guarantees when it returns Equal and one side is a workspace), the comparator first orders by lockfile.workspace_paths[name_hash] and only falls back to package name on a path tie or missing map entry. DepSorter drives sort_unstable_by in lockfile/Tree.rs:725 for the hoisted linker's tree builder and also orders the node-dep queue in isolated_install.rs:664. Two new tests in bun-install-registry.test.ts exercise both path-vs-name orderings, a peer dependent, reinstall from a saved lockfile, and --frozen-lockfile.
Security risks
None. This is a comparator tie-break over trusted lockfile data (workspace name hashes → relative-path strings already in the string buffer). No new parsing, no new I/O, no new user-controlled input paths.
Level of scrutiny
Medium-high. The Rust change is 14 lines and mechanically simple, and the if let (Some, Some) guard makes it a no-op when the map isn't populated. But it is a deliberate behavioral change to the hoisted linker's root-slot winner: every workspace project whose members' package-name order differs from their path order will get a different node_modules layout after this lands. That is the fix for #9838 and matches npm, but it's the kind of ecosystem-facing compat decision a maintainer should sign off on rather than a bot.
Other factors
Prior review feedback has all been addressed: the code comment was trimmed to the two-line npm-compat invariant (my earlier inline comment and the comment-cop flag are resolved), subprocess tests now drain stderr/exited via Promise.all, and the author posted an empirical check that npm keys on lexicographic path (not workspaces array order) plus a note that bun.lock already serializes workspaces in path order via workspace_sort_buf, so --frozen-lockfile stays stable. The bug-hunter found nothing. The remaining reason to defer is purely the blast-radius judgment call on a core install-layout change, not any concrete defect.
|
CI status: the diff is green where it ran; the red is all build-infra.
No failure across any build touches |
|
Build #80897 update: x64-asan lane built and ran tests green, including the new The other 9 red lanes are all |
When two workspaces depend on conflicting versions of the same package, the hoisted linker picks whichever workspace it processes first for the root node_modules slot. DepSorter ordered workspace entries by package name, so a workspace named `@libs/lib` would beat `my-app` even though its directory (`libs/lib`) sorts after `apps/app`. npm orders workspaces by relative path, so the app wins root there and transitive peer dependents dedupe onto it. Sort workspace-behavior dependencies by their workspace path before falling back to name, which matches npm and stops @nestjs/core and similar peer-shared packages from being nested under every consumer. Fixes #9838
dep.version.workspace() does not survive clone_with_different_buffers when the literal is empty (bun.lock load path), so the sort key disagreed between the pre-clean and post-clean lockfiles and --frozen-lockfile failed. lockfile.workspace_paths is populated on every construction path (fresh parse, bun.lock load, migration) and is copied into the cloned lockfile before resolve() runs, so use that as the key instead. Add --frozen-lockfile coverage to the regression test.
…e is stable On load from bun.lock, optional peer edges were bound via the pkg_map path walk, so when process_subtree later visited them the target was already resolved and got enqueued immediately. A fresh resolve leaves optional peers at invalid_package_id until process_subtree binds them via ResolveLater/ResolveReplace, which enqueues them later (via the real dependent that brings the package in). With DepSorter now ordering workspaces by path, this asymmetry surfaced as a different BFS order on the two sides of the frozen-lockfile compare on the #9838 repro (ts-jest's optional @jest/transform peer reached root before @cspotcode/source-map-support on the load side only, flipping which @jridgewell/trace-mapping won the root slot). Skip optional peers in both resolution-binding loops in parse_into_binary_lockfile; process_subtree already handles them identically on both sides. Extend the path-order test with optional-peer-deps to exercise the skip on the --frozen-lockfile path.
757041b to
0ffba3c
Compare
|
Rebased onto main and added a second fix in 0ffba3c: Verified on the reporter's workspace: |
…hots The root-deps loop is the third sibling of the workspace and per-package loops; enqueue_dependency_with_main_and_success_fn returns early for optional peers before reading is_root, so root optional peers are left unresolved on fresh resolve the same as the other two paths. Drop the redundant inline comment (now covered by the is_deferred_peer doc) and reword that doc for the optional-peer half. The next-pages lockfile snapshots record which DependencyID owns each tree slot; jiti's slot is now owned by tailwindcss's real dependency edge instead of eslint's optional peer edge, so the id moves 434 -> 937 (same package_id).
There was a problem hiding this comment.
Beyond the inline nit: I checked the next-pages snapshot delta — jiti's tree binding moved from dep id 434 (an optional-peer * edge) to id 937 (the prod ^1.21.6 edge), same package_id: 316. That's the expected consequence of leaving optional peers unresolved on load; the installed package is unchanged.
Extended reasoning...
The snapshot churn in the three next-pages .snap files is the only place this PR touches existing test expectations, so I traced it: id 434 is {optional: true, peer: true, literal: "*"} for jiti, id 937 is {prod: true, literal: "^1.21.6"} for jiti, and both map to package_id: 316. Before this PR the load path bound the optional-peer edge via the pkg_map walk; now it's skipped and process_subtree picks the prod edge instead — same resolved package, different binding edge. Not approving because this changes hoisted-linker ordering for every workspace install and should get a maintainer's sign-off, but the snapshot change itself is benign.
All three callers skip is_optional_peer() before calling, so the clause was dead. Keep the contract as a debug_assert! and tighten the doc.
There was a problem hiding this comment.
No issues found this pass — all four earlier inline findings have been addressed (comment trimmed to the npm-compat invariant, root-deps loop now skips optional peers alongside its two siblings, is_deferred_peer doc updated and the now-unreachable clause replaced with a debug_assert).
That said, this changes two load-bearing install behaviors at once — workspace hoist ordering in DepSorter (which the isolated linker also uses) and optional-peer binding on bun.lock load — and the next-pages snapshot delta shows it reshapes real trees, not just the synthetic fixtures. Worth a maintainer sign-off on the approach before merge.
What was reviewed:
DepSorter::is_less_than— path lookup falls back to name compare whenworkspace_pathsmisses; only fires inside theBehavior::cmp == Equalworkspace group.- All three resolution-binding loops in
parse_into_binary_lockfilenow guardis_optional_peer()symmetrically;is_deferred_peerhas no other callers. - New tests cover both path orderings, re-install from saved lockfile,
--frozen-lockfile, and that a peer dependent gets no nested copy.
Extended reasoning...
Overview
Two coupled changes to bun install:
DepSorter(src/install/lockfile.rs) — when both sides of a comparison are workspace-behavior deps, compare bylockfile.workspace_paths[name_hash]before falling back to name. This flips which workspace's direct dep wins the rootnode_modulesslot when workspace paths and package names sort differently, to match npm.parse_into_binary_lockfile(src/install/lockfile/bun.lock.rs) — skipis_optional_peer()edges in all three resolution-binding loops (root, workspace, per-package) so load-from-lockfile leaves them unresolved forprocess_subtree, matching fresh resolve.is_deferred_peerdrops its now-unreachable optional-peer clause in favor of adebug_assertand a reworded doc comment.
Plus two new tests in bun-install-registry.test.ts and four one-line id updates in the next-pages lockfile snapshots (jiti's tree-slot owner shifts from eslint's optional-peer edge to its real dependency edge; package_id unchanged).
Security risks
None identified. No untrusted-input parsing, no path construction from external data, no auth/crypto. The change reorders an internal comparator and skips a binding step; inputs are already-parsed lockfile structures.
Level of scrutiny
High. Hoist ordering determines which package version lands at root and therefore what every peer-dependent sees — the #9838 repro shows the previous order broke NestJS DI in a real monorepo. Changing it is a user-visible behavioral shift for any workspace where path order ≠ name order. The optional-peer skip is a second behavioral change to lockfile-load semantics whose only observable justification is --frozen-lockfile stability under the new sort order; whether it has side effects under other tree shapes is the kind of thing a maintainer familiar with process_subtree / install_peer should confirm. DepSorter is also used by the isolated linker (isolated_install.rs:664 per the author's note), so the ordering change is not hoisted-only.
Other factors
- All four of my earlier inline findings (comment bug-history, missing root-loop guard, stale
is_deferred_peerdoc, dead optional-peer clause) are resolved in the current head (d11647e). - Test coverage is good: both path orderings, install from a workspace subdir, re-install from saved lockfile,
--frozen-lockfileafter install, and a peer-dependent nesting assertion. Subprocess draining usesPromise.allper harness convention. - The snapshot delta (
id: 434 → 937, samepackage_id: 316) confirms the optional-peer skip affects real dependency trees beyond the test fixtures — expected given the mechanism, but reinforces that this isn't a no-op outside the repro. - No human maintainer has reviewed yet; CI has been green on the lanes that built (infra timeouts elsewhere).
Deferring so someone who owns the install/lockfile code can sign off on matching npm's path-order semantics and on leaving optional-peer edges unbound at load time.
|
Build #87034 on d11647e: 188 jobs passed. The two
The rest are the |
### Repro On a pristine checkout of main, with the released canary (1.4.0-canary.1, 9008ae7): ```sh cd test bun install --frozen-lockfile # error: lockfile had changes, but lockfile is frozen bun install --lockfile-only && git diff --stat bun.lock # re-hoists jsbn bun install --frozen-lockfile # still fails on the re-saved file ``` The re-save flips the top-level `jsbn` from 1.1.0 to 0.1.1, drops `ecc-jsbn/jsbn` and `sshpk/jsbn`, and adds `ip-address/jsbn`. Re-saving does not help: `--frozen-lockfile` rejects the re-saved lockfile too, so a project with this shape cannot pass `--frozen-lockfile` at all. 1.3.14 accepts both layouts. Any project with an optional peer whose target is reached through a deeper dependency, plus a version conflict somewhere under that target, hits this (here `mongodb` has an optional peer on `socks`, `socks -> ip-address -> jsbn@1.1.0`, and `sshpk` wants `jsbn@0.1.1`). The new registry fixtures are a small version of the same graph, and the same three commands fail on them the same way. ### Cause Loading `bun.lock` binds every optional peer to the package next to it (the path walk in `bun.lock.rs`) and then hoists. Since #35681, `Package::clone` writes `invalid_package_id` into optional peer slots while cleaning, so the hoist in `Cloner::flush` runs with those edges unbound. Hoisting is breadth-first, and the two runs queue the peer target's subtree at different times: with the edge bound, the target is placed when its dependent is (`socks` from `mongodb`, at depth 1) and its subtree is walked right after; unbound, the target is only placed when the first real edge reaches it (`socks-proxy-agent`, several levels down) via `ResolveReplace`. Whatever conflicts under that subtree (`jsbn`) is hoisted differently, so the tree built at load time and the tree built by the clean differ. `--frozen-lockfile` compares exactly those two trees (`Lockfile::eql`), which is why it fails on an unchanged project and keeps failing after a re-save: the file is written from the clean's tree, but loading it binds the peers again and builds the other one. The same thing makes an unrelated `bun add` rewrite these entries. #35681 described this as a one-time rewrite; it is not, because the load side still binds the slots. ### Fix Three pieces, all in the lockfile tree builder: 1. **Clean keeps the bindings.** `Package::clone` defers optional peer slots to `Cloner::optional_peers` instead of clearing them; `Cloner::flush` binds them once the clone queue has drained, and only when the target was actually cloned, i.e. some non-peer edge still reaches it. A target held only by peer slots is still never cloned, so what #35681 fixed (`bun remove` leaving the package behind) stays fixed and its tests still pass. For a surviving target this is what 1.3.x did (`Package.clone` in Zig copied every slot), so lockfiles written by 1.3.x are fixed points: `test/bun.lock` passes `--frozen-lockfile` and re-saves byte-identically with this branch, without touching the file. 2. **A kept binding has to be the one the tree expresses.** The hoister places a bound optional peer like any dependency, except that when another version of the target already holds the slot next to the dependent and the peer range accepts it, the dependent dedupes onto that version. In that case the binding now moves to that version too (`HoistDependencyResult::Rebind`, in the saved tree only). That is what node resolution finds, what the path walk binds on the next load, and what the isolated linker keys the dependent's store entry by, so the install that writes `bun.lock` and the next install from it link the same entry. Without this, a carried-over binding could survive in memory for one run while the saved tree said otherwise (the isolated linker would then link against one version and a reinstall against the other). Required peers are untouched: they keep the resolver's version, which `bun.lock.rs` reproduces by version since #32182. So the rule for an optional peer is unchanged from main: it is bound to whatever ends up next to it. The carry-over only matters when the bound package is still the one placed there, and then it keeps its place instead of being re-derived. 3. **Fresh installs settle in the same pass structure a reload has.** When a peer is bound late in a hoist pass (`ResolveReplace`), the pass is repeated, until a pass binds nothing late. The last pass built the tree from bindings it had up front, which is what a reload does, so a fresh install writes the same file a reload would write instead of one that converges on the next re-save. Binding one peer can move a dependent and put a target within reach of a second peer that the previous pass could not bind (shape 2 of the fixtures: the target is hidden behind a bundled copy of itself until the first binding hoists its dependent out), which is why this is a loop rather than one extra pass. Each pass that repeats filled at least one empty slot and no pass empties one, so it ends after at most one pass per optional peer; in practice lockfiles loaded from disk take one pass and fresh installs one or two. `filter` (the install-time tree) runs after this and stays single-pass. Why keep the binding rather than also stop binding at load time (the other way to make the two sides agree, which #35571 carries as a side change)? Re-deriving everywhere rewrites every existing lockfile with this shape once, failing `--frozen-lockfile` in CI on the bun upgrade (that approach had to regenerate the `next-pages` lockfile snapshots; this branch regenerates nothing), and it rebinds a peer to a different version of its target on unrelated `bun add`s. The binding recorded in the lockfile is a resolution like any other; keeping it while the target exists and the tree can express it is what the lockfile is for, and it is what 1.3.x did. Lockfiles written by builds with #35681 (the other placement) are accepted as well and converge on their next re-save; the parameterized test covers both placements. #37350 (bundled dependency's optional peer bound across the bundle root at load) is a different shape of the load/clean mismatch; its test passes on this branch too, and it is still worth landing on top because it stops that edge from being bound at all. ### Verification Registry fixtures `optional-peer-hoist-*` (generated by `create-optional-peer-hoist-packages.ts`, which documents both shapes; every tarball is a single `package.json`, plus the bundled copy in `target@3.0.0`). Tests added to `test/cli/install/bun-lock.test.ts`, each with what fails without the fix: - fresh install, then `--frozen-lockfile` passes and `--lockfile-only` re-saves byte-identically (before: `error: lockfile had changes, but lockfile is frozen` on the file the install just wrote) - shape 2: a fresh install writes the settled layout (`tail@2.0.0` at the root) and re-saves identically (before, and with a fixed two passes: `tail@1.0.0` at the root and the first re-save rewrites it) - `--frozen-lockfile` accepts a hand-written `bun.lock` in either placement (both rejected before) - adding `provider` keeps `consumer` bound to `target@1.0.0`, `target@2.0.0` nests under `provider`, `--frozen-lockfile` passes, re-save identical (before: rebound to 2.0.0 and the existing entries re-hoisted) - the same with `provider` aliased to sort first, under the isolated linker: `target@2.0.0` takes the root, and the store entry `consumer` is linked to by the writing install is the one a `--frozen-lockfile` reinstall from scratch links to as well (fails with piece 1 alone: the writing install still links the `target@1.0.0` variant) The four tests from #35681 still pass. Also run with the debug build: `bun-lockb`, `migrate-bun-lockb-v2`, `hoist`, `lockfile-only`, `lockfile-version-2`, `isolated-install` (including the #32182 peer stability tests), `bun-remove`, `bun-add`, `bun-update`, `bun-workspaces`, `overrides`, `catalogs`, `public-hoist-pattern`, `config-version`, `bun-pm`, `bun-pm-why`, `bun-install-registry`, `bun-install` and the `migration/` suites (lockfile snapshots unchanged). The only failures are tests needing bitbucket/gitlab/external network access, which fail identically with the released build in this environment. `cd test && bun bd install --frozen-lockfile` passes on the committed `test/bun.lock`, and `--lockfile-only` leaves it unchanged.
This branch has not been deployed
3 participants
What
When two workspaces depend on conflicting versions of the same package, the hoisted linker's
DepSortervisited workspace entries in package-name order, while npm visits them in relative-path order. Whichever workspace is visited first wins the rootnode_modulesslot; the other gets a nested copy.In the #9838 repro (a NestJS monorepo) that difference breaks DI:
@nestjs/coredepapps/nestjs-boilerplatenestjs-boilerplate^10.0.0libs/nest-init-app-fastify-adapter@libs/nest-init-app-fastify-adapter^9.4.0@libs/...sorts beforenestjs-boilerplateby name, so bun hoisted@nestjs/core@9.4.3to root. Every root-level package that peers on@nestjs/core>=10(nestjs-omacache,@nestjs/swagger,@nestjs/testing) then got its own nested@nestjs/core@10.4.22, so the app'sReflectorclass and the guard'sReflectorclass had different identities:npm sorts by path, so
apps/...wins root (@nestjs/core@10.4.22), the^9copy nests only under the one lib that needs it, and the peer dependents all share the root copy.Fix
DepSorter(src/install/lockfile.rs)When both sides are workspace-behavior entries, compare by
lockfile.workspace_paths.get(&name_hash)before falling back to name. That map is populated on every construction path (fresh parse,bun.lockload, migration) and is copied into the cloned lockfile beforeresolve()runs inclean_with_logger, so the sort key is stable across fresh install, re-install from a lockfile, and--frozen-lockfile.Behavior::cmpalready puts workspaces first; this only changes the tie-break within that group.parse_into_binary_lockfile(src/install/lockfile/bun.lock.rs)The sort-order change exposed a pre-existing
--frozen-lockfileasymmetry: on load frombun.lock, the three resolution-binding loops (root, workspace, per-package) bound optional-peer edges via thepkg_mappath walk, but fresh resolve leaves them atinvalid_package_iduntilprocess_subtreebinds them. With the new BFS order the two sides enqueue@jridgewell/trace-mappingat different points on the #9838 repro, so--frozen-lockfilereported "lockfile had changes". Skipis_optional_peer()in all three loops so load matches fresh resolve, and update theis_deferred_peerdoc accordingly.Verification
With the issue's empty_starter.zip, after
bun install --linker=hoisted:before:
after (matches
npm install --legacy-peer-deps):and
bun install --frozen-lockfileimmediately after reports "no changes".New tests in the
hoistingsuite cover both path orderings, re-install from the saved lockfile,--frozen-lockfileafter install, and assert that a peer dependent (strict-peer-deppeering onno-deps@^2.0.0) gets no nested copy. Thenext-pageslockfile snapshots are regenerated (jiti's root tree slot is now owned by its real dependency edge instead ofeslint's optional-peer edge; samepackage_id).Fixes #9838.
no test proof · iteration 6 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-install-registry.test.ts