Conversation
…e of the same name A root dependency on the registry package that has a workspace's name replaces the root's workspace dependency when the workspace's version is not in its range, because both would install in node_modules/<name>. An aliased dependency installs in node_modules/<alias>, so there is no folder to share. It still replaced the workspace dependency, and the workspace and its own dependencies left the install. The replacement now needs the dependency to take the workspace's folder name.
|
Updated 3:56 PM PT - Sep 19th, 2026
✅ @robobun, your commit b05429866723411767608c33db3a7c5e07c0f140 passed in 🧪 To try this PR locally: bunx bun-pr 43567That installs a local version of the PR into your bun-43567 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughThe dependency parser now preserves a workspace when a conflicting registry dependency uses an alias. Tests cover hoisted and isolated linkers, versioned and versionless workspaces, lockfile behavior, repeated installs, and frozen-lockfile installs. ChangesWorkspace alias handling
Suggested reviewers: Priority: ⬇️ Low 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the sibling linked_workspace_path branch at src/install/lockfile/Package.rs:1887: an alias whose range the workspace does satisfy (e.g. published: "npm:no-deps@^3.0.0") still links node_modules/published to the workspace because that lookup keys on the real name, not the alias — that is pre-existing behavior on main and not changed by this diff, so it is not a regression here.
Extended reasoning...
The diff narrows one else if in the Tag::Npm arm so the workspace-override only fires when the dependency's alias equals the package's real name. The satisfying-range branch directly above it (linked_workspace_path) still matches on name_hash only, so an alias whose range happens to include the workspace version resolves to the workspace path rather than the registry package; I traced this and confirmed it is untouched by the PR and behaves identically on the base commit, so it is an adjacent pre-existing inconsistency rather than something introduced here. The new test matrix covers both linkers, version present/absent, and the bun add path, and asserts whole lockfile maps with toEqual, so it can fail for the right reason; the confirmed inline finding about older lockfiles failing --frozen-lockfile once after upgrade is already posted and is the item a human should weigh.
|
Status: ready for review. Reproduced on bun 1.4.3-canary.1 (367d939) and with Manual repro: a root with The case where the root dependency has the workspace's own name is not changed here. It needs a decision: #43568. |
…ses the workspace A bun.lock written before the alias fix does not list the workspace. A frozen install rejects it with 'lockfile had changes', and a plain install adds the workspace.
There was a problem hiding this comment.
I re-reviewed after the second commit and found no bugs in the change itself; since the PR now deliberately pins a one-time --frozen-lockfile failure for lockfiles an older bun wrote for this shape (rather than migrating them), a maintainer should confirm that trade-off before merging.
What was reviewed:
- The narrowed guard at
src/install/lockfile/Package.rs:1910: for a non-aliased depnpm().nameis the package.json key, soexternal_alias.hash == name_hashholds and the existing override is unchanged; a self-alias like"no-deps": "npm:no-deps@ 1.0.0"still overrides, and only a differently named alias falls through. - Grepped
src/installfor other sites that replace or drop a workspace dependency byname_hash— this loop is the only one, so there is no sibling site left unfixed. - The 6 new rows cover both linkers,
bun installandbun add, the versionless-workspace shape, no-op re-install and frozen install; the new "out of date bun.lock" test asserts the exact frozen-lockfile error and that a plain install repairs the lockfile.
Extended reasoning...
Overview
The PR changes one condition in Package<u64>::parse_dependency (src/install/lockfile/Package.rs:1910): the branch that replaces the root's implicit workspace dependency with a same-named npm dependency now also requires external_alias.hash == name_hash, i.e. the dependency must install into the workspace's own folder. An alias such as "published": "npm:no-deps@ 1.0.0" no longer removes the no-deps workspace and its transitive dependencies from the lockfile. test/cli/install/bun-workspaces.test.ts adds a describe.each over both linkers with three rows (out-of-range workspace version, versionless workspace, alias added via bun add) and, in the second commit, one test that hand-edits bun.lock to the shape an older bun produced and asserts --frozen-lockfile rejects it while a plain install repairs it.
Security risks
None identified. The change only affects which dependency entry the root package keeps for a workspace name collision; it does not touch path handling, integrity verification, network access, or credential flow. The tests use the Verdaccio harness and no public network.
Level of scrutiny
The Rust change is small and I verified the semantics of the new clause rather than trusting the names: dependency::parse_with_tag sets npm.name to the alias only for npm:-prefixed specs, otherwise to the key itself, so the equality check is exactly "alias differs from resolved name". I also confirmed no other site in src/install performs the same workspace replacement, so the bug class is contained to this loop. What keeps this from an approve is the product-level trade-off the author chose in the second commit: a lockfile written by a previous release for this shape now fails once under --frozen-lockfile and is pinned as intended behavior in a test instead of being migrated at load time. That is a user-visible compatibility decision, and the PR description also notes two open PRs whose tests assume the old drop behavior plus an unresolved sibling case ("no-deps": "1.0.0" in the root) tracked separately. A maintainer should weigh those explicitly.
Other factors
The test matrix is reasonably complete: both linkers, bun install and bun add, the versionless-workspace shape that already passed on main (regression guard), a no-op re-install assertion via savesLockfile: false and byte-equal lockfile text, and a frozen install at the end. Assertions use toEqual on whole objects (lockfile package resolutions, installed package.json contents) and toContain for the exact frozen-lockfile error. The multi-agent hunt ran to a dry streak with no findings, and the second commit addressed the compatibility point raised in my earlier inline comment by making it explicit rather than silent.
Problem
"published": "npm:no-deps@1.0.0"plus a workspaceno-deps@3.0.0:bun installexits 0 and drops the workspace. It leavesbun.lock, and its own dependencies are not installed.bun add published@npm:no-deps@1.0.0printsRemoved: 1.Tag::Npmarm ofPackage::parse_dependency(src/install/lockfile/Package.rs:1910). When the workspace's version is not in the range, the registry dependency replaces the root's dependency on the workspace. The match uses the package name behind the alias, not the folder.versionfield skips that branch, so main already keeps both.Fix
external_alias.hash == name_hash). The replacement settles one folder,node_modules/<name>, and an alias installs innode_modules/<alias>.bun.lockalready records this state for a workspace without a version."no-deps": "1.0.0"in the root still replaces the workspace. That case needs a decision: bun install drops a workspace and its dependencies when the root depends on the registry package of the same name #43568.bun.lockfrom an older bun misses the workspace.--frozen-lockfilefails on it once (lockfile had changes), and a plainbun installrepairs it.test/cli/install/bun-workspaces.test.ts, 7 new rows, 5 fail on bun 1.4.3-canary.1. Notes: other suites, self-review.Background
workspacesentry. A workspace that nothing depends on leaves the lockfile.linked_workspace_path)."published": "npm:no-deps@1.0.0"is an alias: the packageno-depsinstalls innode_modules/published.Notes
Result with this change (hoisted linker; the isolated linker links
a-depinpackages/no-deps/node_modules):A second
bun installsaves nothing, andbun install --frozen-lockfilepasses.Lockfiles written before this change. A
bun.lockthat an older bun wrote for this shape does not have the workspace. With this changebun install --frozen-lockfilefails once on it withlockfile had changes, but lockfile is frozen. A plainbun installadds the workspace and its dependencies and saves. The file was missing a workspace, so this is the same result as for a workspace folder added without a new lockfile (stock bun prints the same error there). The testbun.lock without the workspace that a root alias used to drop is out of datepins both steps. A load-time migration is not possible: a frozen install cannot add the workspace's dependencies.Self-review (by hand). Checked: no other code on main repeats the replacement rule (the
bun.lockloader adds one root dependency perworkspacesentry and has no such rule).bun add,bun update,bun update --latest,bun remove,bun pm ls,bun whyandbun outdatedon the fixed shape, each followed by--frozen-lockfile. A registry package with a peer range that the workspace satisfies binds to the workspace again (on the loopback fixture stock bun warnsincorrect peer dependency "host@1.0.0"there, because the workspace is gone).linkWorkspacePackages = false: the alias resolves from the registry and the workspace stays. Two aliases of the same package. An alias named like a second workspace: that folder conflict is the same as on main, and the first workspace is no longer dropped. One cost found: the frozen-lockfile result above.Origin. The replacement rule is from #11177. Its only test for a dependency with a workspace's name is
bun add bar@0.0.7inside another workspace, which does not reach this branch.Open PRs with tests that assume the drop. #37248 has the test
aliased npm dependency colliding with a member's name. It asserts thatbun.lockhas nono-deps@workspace:entry for"my-alias": "npm:no-deps@1.0.0"plus a workspace without a version. main keeps the workspace in that shape today. #43468 has the history "a root alias takes the registry package of that name". Both need a new expectation for the alias rows if this lands first.Other package managers on the alias fixture (loopback registry, root
"published": "npm:host@1.0.0", workspacehost@1.5.0with a dependencyleaf). npm 11.16, yarn 1.22 and yarn 4.18:node_modules/publishedis the registry copy,node_modules/hostlinks the workspace,leafis installed. pnpm 12.4:node_modules/publishedis the registry copy,packages/hoststays an importer,leafis inpackages/host/node_modules.Suites run with the debug build:
bun-workspaces(89),bun-add(71),bun-update(159),bun-lock(40),bun-remove(14),overrides(7),catalogs(89),isolated-install(85),bun-install-registry(281). All pass.bun-installhas 13 failures. All are bitbucket, gitlab or external URL tests, and the same 13 fail with the released build.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-workspaces.test.ts