Repository navigation
Conversation
… package name `bun add` sorted the target dependency group of package.json each time it added or replaced an entry. So `bun add zeta@./zeta`, for an entry that the group already has, moved every entry of a group that was not in sorted order. The rebuild of the group now sorts only when a request gives the group a package name that it did not have. A request that names an entry of the group changes that entry in its place. A group that gets a new name is sorted as before.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 19 seconds. View limit details
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 |
|
Status Reproduced on main
CI: Buildkite build #129695 passed. Four tests outside PR: #46253 |
|
Updated 8:56 PM PT - Oct 20th, 2026
✅ @robobun, your commit 99f99be92dd0dde362dae95a7c968ce5e8a98c77 passed in 🧪 To try this PR locally: bunx bun-pr 46253That installs a local version of the PR into your bun-46253 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes user-visible bun add output (when a group is re-sorted) and the PR itself lists two intentional downsides, a human look at that product decision would still be worthwhile.
What was reviewed:
- The
swap(j, k)+remove(k)+k = if j > k { j - 1 } else { j }rewrite inedit(): traced bothj < kandj > k, and the duplicate-key case where the reverse loop matches more than once — every unvisited index stays unvisited-once andkkeeps pointing at the re-keyed entry. gained_nameacross aliased (new empty slot), non-aliased resolved (!replaced_declared), unresolved literal, andbun updatepaths — each consumer matches the table in the description; the unresolved./missingkey landing last instead of sorted is a pre-existing exit-0 bug, not introduced here.- The new test matrix: hermetic folder packages, pipes drained concurrently,
using tempDir; the one serialit.eachuses the file-level registry/package_dir while the concurrent siblings use their own temp dirs, so they do not share state.
Extended reasoning...
The change touches one Rust function, edit() in src/install/PackageManager/PackageJSONEditor.rs (+20/-8), gating the existing alphabetize call behind a new gained_name flag and reordering the stale-duplicate removal so the re-keyed entry keeps the declared entry's position, plus 315 lines of tests in test/cli/install/bun-add.test.ts. It touches no security-sensitive surface (no auth, crypto, path handling from untrusted input, or network). The index arithmetic and the flag's data flow check out and the test matrix covers the stated variants, so no findings; the reason for deferring rather than approving is that this is a deliberate behavior change to what bun add writes, with downsides the author acknowledges (re-adding a known name no longer sorts; an unresolved literal key now lands last), which is a maintainer judgment rather than a correctness question. No CODEOWNERS entry covers the changed files and there are no third-party review events on the PR.
Related to #11536
Problem
bun addsorts the group it writes to also when it adds no package:bun add zeta@./zetarewriteszeta, beta, alphaasalpha, beta, zeta.edit(src/install/PackageManager/PackageJSONEditor.rs:973) rebuilds the group for each resolved request and always sorts it.Fix
test/cli/install/bun-add.test.ts(34 new cases, 22 fail on main). Release builds of main and this branch differ only in those cases overtest/cli/install/.Background
editruns before and after the install. Until the install gives a name, the key ofbun add ./folderis./folder.bun removeandbun update <name>keep the order (install: consolidate the open package manager pull requests #44968, install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333).Downsides
bun add <a package the group has>no longer sorts the group.bun add --optional ./missing, on main too) is now the last entry. Main sorts the group..textgrows by 768 bytes. Instructions inedit:bun add <new name>26,736 to 25,774,bun update <name>5,656 to 5,652.Notes
What counts as a new package name
-d,--optional,--peeror--save-prodmoves in from another group.When every request names an entry that the group has, the group keeps its order. The tests cover
bun add <name>@<spec>,bun install <name>@<spec>, the group flags,-g,--filter,--catalog,bun link <name> --save, and a folder of a package that the group has.bun addstill prints the whole file. A file that is not in the format Bun prints (comments, CRLF, objects on one line) is printed again when a value changes, as on main. Writes that keep the text are #45812.The ways into the rebuild of a group
bun-add.test.ts)bun add gamma@./gamma: a new packagebun add -d delta@./deltabun add zeta@./zeta: an entry that is not a stringbun add zeta@./zeta,bun add zeta@./zeta2bun add ./zeta2bun add ./gammabun add --peer ./zeta2: another folder of an optional peerIn the last row the entry with the name takes the place of the entry it replaces. Main removed the declared entry and sorted.
npm, pnpm and yarn sort all four groups on each save. This PR does not follow them for a group that gets no new name, as
bun remove(#44968) andbun update <name>(#38333) already do not. There is no flag. A group in sorted order gets the bytes of main, with the one exception under Downsides.Not in this PR
bun updateremoves comments in package.json #11536. install: save an edited package.json over its own text #45812 handles them. The two PRs share no file.alphabetize_appended(add_catalog.rs) andedit_trusted_dependenciesalready sort only when they add a name.bun add -d ./zeta zeta@./zeta2over a dev entry and a peer entryzeta: ./zetaleft package.json at./zetawhile bun.lock and node_modules gotzeta2. Main writes./zeta2, and this branch does too. That change also removes no sort that the condition does not remove, and open Remove dead code from bun_bundler, the JS parser, the help command, and PackageJSONEditor #44687 edits the same condition.Bugs that main has and this PR does not change
No tracking issue is filed from this PR for these.
bun add --optional ./missing(the folder does not exist) exits 0 and saves"./missing": "./missing"in package.json and bun.lock. 1.4.2 and 1.4.3 exit 1 witherror: Invalid dependency name "./missing"and write nothing. On this branch that key is the last entry of its group. Main sorts the group, so the key is first.bun add ./zeta zeta@./zeta2over a declaredzeta: ./zetaexits 0 and saves"./zeta": "./zeta"besidezeta. The key is first on main and last on this branch.bun add --peer ./zeta2over an optional peerzetawrites"zeta": "./zeta2"two times intopeerDependenciesof bun.lock, on 1.4.3, on main and on this branch. The four cases for that command assert package.json only.Measurements
Release builds of main
91e7e3b140and of this branch, same toolchain. The project has local folder packages, so no command needs a registry. The group is written aszeta, beta, alpha.Entries that change in package.json per command (
difflines), main to this branch:bun add zeta@./zetabun add zeta@./zeta2bun add -d zeta@./zeta(group in devDependencies)bun add ./zeta2bun add ./zeta2(zeta only in devDependencies)bun add --peer ./zeta2(over an optional peer)bun add gamma@./gammabun add ./gammabun add zeta@./zeta gamma@./gammabun add ./zetaCounts per command (gdb breakpoints on the profile builds), main to this branch:
editbun add zeta@./zetabun add zeta@./zeta2bun add ./zeta2bun add gamma@./gammabun add ./gammabun add --optional ./missingbun add ./zetabun update zetabun add zeta@./zetaon the group that is not sorted: the inode and the timestamps of package.json do not change on this branch, andcmpfinds no difference. The tree is still rebuilt. The write is skipped because the printed bytes are the bytes of the file.editthat runs after the install, three runs each:bun add zeta@./zeta10 to 8,bun add gamma@./gamma10 to 8.edit, both calls summed, ten folder dependencies that are not sorted, minimum of 5 runs (largest spread 130):bun update zeta5,656 to 5,652,bun add zeta@./zeta19,109 to 16,174,bun add gamma@./gamma26,736 to 25,774,bun add ./gamma29,773 to 28,847..text81,665,765 to 81,666,533 bytes (+768).edit14,090 to 14,713 bytes..rodatadoes not change.bun add --optional ./missing, the first bug above) and 0 of 17 bun.lock. With the group not sorted, bun.lock is the same in all 17.test/cli/install/, release builds: main 12,127 pass and 23 fail, this branch 12,149 pass and 1 fail. The 22 that differ are the new cases. The 1 that fails on both issemver.test.ts, which cannot find the packagesemveron this machine.Tests
bun-add.test.ts, "the order of a dependency group after bun add": 34 cases, each compares the text of package.json. 22 fail on main. The 12 others pin what must not change.bun install --frozen-lockfile. The other 2 cases use the registry of the test file for the command of thebun updateremoves comments in package.json #11536 thread,bun install --dev <alias>@npm:<package>@<version>.bun-add.test.ts253 pass.bun-link,bun-remove,bun-update,bun-add-filterandbun-add-catalogpass with a test timeout of 180 s. With the default 5 s some of their tests time out on this machine's debug build.