From 47fcb62aab80f6cd7ade80f13f8edf3eba0649e0 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 03:30:21 +0000 Subject: [PATCH] install: move an existing dependency when bun add is given a group flag When bun add is run with --dev, --optional or --peer and the package is already listed in a different dependency group, remove it from that group and add it to the requested one instead of updating the old entry in place. A bare bun add keeps the existing placement, as npm and pnpm do. A peerDependencies entry is left alone when adding to another group, and an existing devDependencies entry is kept and rewritten to the new range when adding a peer dependency. Removals are ordered so the other entries and root keys keep their order. The --filter and --catalog variants share this path; the three tests in bun-add-filter.test.ts and bun-add-catalog.test.ts that pinned the update-in-place behavior now expect the move, including the bun.lock sync checks after a filtered --peer add. Fixes #5714 Fixes #4852 --- .../PackageManager/PackageJSONEditor.rs | 57 ++++++++++- test/cli/install/bun-add-catalog.test.ts | 13 ++- test/cli/install/bun-add-filter.test.ts | 9 +- test/cli/install/bun-add.test.ts | 95 ++++++++++++++++++- 4 files changed, 164 insertions(+), 10 deletions(-) diff --git a/src/install/PackageManager/PackageJSONEditor.rs b/src/install/PackageManager/PackageJSONEditor.rs index 88773115f6a9..cd90a066007d 100644 --- a/src/install/PackageManager/PackageJSONEditor.rs +++ b/src/install/PackageManager/PackageJSONEditor.rs @@ -976,6 +976,10 @@ pub(crate) fn edit( let mut remaining = updates.len(); let mut replacing: usize = 0; let only_add_missing = manager.options.enable.only_missing(); + // `--dev`/`--optional`/`--peer` moves an existing entry into that group; a bare `bun add` keeps its placement. + let move_to_target = !only_add_missing && dependency_list != DependencyGroup::DEPENDENCIES.prop; + // `(index into updates, existing devDependencies value)` pairs that follow the new peer range. + let mut dev_entries_to_sync: Vec<(usize, *mut E::EString)> = Vec::new(); // There are three possible scenarios here // 1. There is no "dependencies" (or equivalent list) or it is empty @@ -987,7 +991,7 @@ pub(crate) fn edit( let mut i: usize = 0; 'loop_: while i < updates.len() { let request = &mut updates[i]; - // order-insensitive scan: `FOUR` is fine here + // `move_to_target` visits every group; otherwise order-insensitive (bails on first match). 'dependency_group: for list in DependencyGroup::FOUR.map(|g| g.prop) { if let Some(query) = current_package_json.as_property(list) { if matches!(query.expr.data, bun_ast::ExprData::EObject(_)) { @@ -1004,14 +1008,48 @@ pub(crate) fn edit( == dependency::Tag::Catalog }, ); + let in_target_list = + strings::eql_long(list, dependency_list, true); // `bun update ` edits the slot in place; the rebuild below re-sorts the keys. if request.package_id != INVALID_PACKAGE_ID && manager.subcommand != Subcommand::Update - && strings::eql_long(list, dependency_list, true) + && in_target_list && !keep_catalog_reference { replacing += 1; + } else if move_to_target && !in_target_list { + match list { + // A peer entry coexists with every other group. + b"peerDependencies" => {} + // A dev entry coexists with a peer entry and tracks its range. + b"devDependencies" + if dependency_list + == DependencyGroup::PEER.prop => + { + dev_entries_to_sync.push(( + i, + value + .expr + .data + .e_string() + .expect("infallible: variant checked") + .as_ptr(), + )); + } + _ => { + changed = true; + let mut group = query.expr.data.as_e_object(); + let _ = group.properties.remove(value.i as usize); + if group.properties.is_empty() { + let _ = current_package_json + .data + .as_e_object_mut() + .properties + .remove(query.i as usize); + } + } + } } else { if manager.subcommand == Subcommand::Update && options.before_install @@ -1073,6 +1111,9 @@ pub(crate) fn edit( } } } + if move_to_target { + continue 'dependency_group; + } break; } else { // For non-aliased positionals where `get_name()` returns the @@ -1460,6 +1501,18 @@ pub(crate) fn edit( } } } + for (update_index, dev_entry) in dev_entries_to_sync { + if let Some(peer_entry) = updates[update_index].e_string { + // SAFETY: both pointers were captured at the provenance sites described on the loop + // above and stay valid for the same reasons; that loop has released its borrows. + unsafe { + if (*dev_entry).data.slice() != (*peer_entry).data.slice() { + changed = true; + (*dev_entry).data = (*peer_entry).data; + } + } + } + } Ok(changed) } diff --git a/test/cli/install/bun-add-catalog.test.ts b/test/cli/install/bun-add-catalog.test.ts index fdd2154c7ee0..78c3014e72ee 100644 --- a/test/cli/install/bun-add-catalog.test.ts +++ b/test/cli/install/bun-add-catalog.test.ts @@ -699,8 +699,17 @@ describe.concurrent("bun add --catalog", () => { const plainPkg1 = await plain.pkg1(); const catalogPkg1 = await catalog.pkg1(); expect(Object.keys(catalogPkg1)).toStrictEqual(Object.keys(plainPkg1)); - expect(plainPkg1).toStrictEqual({ name: "pkg1", peerDependencies: { "no-deps": "^2.0.0" } }); - expect(catalogPkg1).toStrictEqual({ name: "pkg1", peerDependencies: { "no-deps": "catalog:" } }); + // The peer entry is left alone; --dev adds a devDependencies entry next to it. + expect(plainPkg1).toStrictEqual({ + name: "pkg1", + peerDependencies: { "no-deps": ">=1" }, + devDependencies: { "no-deps": "^2.0.0" }, + }); + expect(catalogPkg1).toStrictEqual({ + name: "pkg1", + peerDependencies: { "no-deps": ">=1" }, + devDependencies: { "no-deps": "catalog:" }, + }); }); test("--peer", async () => { diff --git a/test/cli/install/bun-add-filter.test.ts b/test/cli/install/bun-add-filter.test.ts index 80d66f7ecdc8..714d29b703bc 100644 --- a/test/cli/install/bun-add-filter.test.ts +++ b/test/cli/install/bun-add-filter.test.ts @@ -179,13 +179,13 @@ test.concurrent("-F alias with --dev and --exact", async () => { expect(await pkg(dir, "web")).toStrictEqual(WEB); } - // Same as unfiltered `bun add -d`: an entry that already exists in another list is updated in place. + // Same as unfiltered `bun add -d`: an entry that already exists in another list is moved to the requested one. { const { stderr, exitCode } = await run(["add", "a-dep", "-F", "web", "-d", "-E"], dir); expect(stderr).not.toContain("error:"); expect(exitCode).toBe(0); - expect(await pkg(dir, "web")).toStrictEqual({ name: "web", dependencies: { "a-dep": "1.0.10" } }); + expect(await pkg(dir, "web")).toStrictEqual({ name: "web", devDependencies: { "a-dep": "1.0.10" } }); expect(await pkg(dir, "api")).toStrictEqual({ name: "api", devDependencies: { "a-dep": "1.0.10" } }); expect(await pkg(dir, "root")).toStrictEqual(ROOT); } @@ -2340,12 +2340,11 @@ test.concurrent("a name declared in two groups stays in sync with bun.lock after expect(stderr).not.toContain("error:"); expect(exitCode).toBe(0); - // Same as an unfiltered add: the entry that already exists (in dependencies) is updated in place. + // Same as an unfiltered add: the dependencies entry is dropped and the existing peer entry takes the new range. const api = await pkg(dir, "api"); expect(api).toStrictEqual({ name: "api", - dependencies: { "no-deps": "^2.0.0" }, - peerDependencies: { "no-deps": "*" }, + peerDependencies: { "no-deps": "^2.0.0" }, }); const { workspaces } = await lockfileJson(dir); expect(workspaces["packages/api"]).toStrictEqual(api); diff --git a/test/cli/install/bun-add.test.ts b/test/cli/install/bun-add.test.ts index 423a7fbb8e4a..996098ff0800 100644 --- a/test/cli/install/bun-add.test.ts +++ b/test/cli/install/bun-add.test.ts @@ -754,6 +754,99 @@ it("should add to peerDependencies with --peer", async () => { await access(join(package_dir, "bun.lockb")); }); +describe("should move existing dependency when an explicit group flag is given", () => { + const root = { name: "foo", version: "0.0.1" }; + const pkgJson = (pkg: Record) => JSON.stringify(pkg, null, 2); + + async function add( + initial: Record, + args: string[], + registry: Record = { "0.0.2": {} }, + ) { + setHandler(dummyRegistry([], registry)); + await writeFile(join(package_dir, "package.json"), JSON.stringify(initial)); + const { stdout, stderr, exited } = spawn({ + cmd: [bunExe(), "add", ...args], + cwd: package_dir, + stdout: "pipe", + stdin: "pipe", + stderr: "pipe", + env, + }); + const [err, , exitCode] = await Promise.all([stderr.text(), stdout.text(), exited]); + expect(err).not.toContain("error:"); + expect(err).toContain("Saved lockfile"); + expect(exitCode).toBe(0); + return await file(join(package_dir, "package.json")).text(); + } + + it.each([ + ["dependencies", "devDependencies", "--dev"], + ["dependencies", "optionalDependencies", "--optional"], + ["dependencies", "peerDependencies", "--peer"], + ["devDependencies", "optionalDependencies", "--optional"], + ["optionalDependencies", "devDependencies", "--dev"], + ["optionalDependencies", "peerDependencies", "--peer"], + ])("%s -> %s with %s", async (from, to, flag) => { + expect(await add({ ...root, [from]: { BaR: "^0.0.2" } }, [flag, "BaR"])).toBe( + pkgJson({ ...root, [to]: { BaR: "^0.0.2" } }), + ); + }); + + it("keeps the remaining entries of the source group in their original order", async () => { + const dependencies = { monkey: "^0.0.2", BaR: "^0.0.2", boba: "^0.0.2", "depends-on-monkey": "^0.0.2" }; + expect(await add({ ...root, dependencies }, ["--dev", "BaR"])).toBe( + pkgJson({ + ...root, + dependencies: { monkey: "^0.0.2", boba: "^0.0.2", "depends-on-monkey": "^0.0.2" }, + devDependencies: { BaR: "^0.0.2" }, + }), + ); + }); + + it("keeps the other root keys in their original order when the source group becomes empty", async () => { + const rest = { type: "module", scripts: { test: "true" }, license: "MIT" }; + expect(await add({ ...root, dependencies: { BaR: "^0.0.2" }, ...rest }, ["--dev", "BaR"])).toBe( + pkgJson({ ...root, ...rest, devDependencies: { BaR: "^0.0.2" } }), + ); + }); + + it("moves several packages out of the same group", async () => { + expect(await add({ ...root, dependencies: { BaR: "^0.0.2", monkey: "^0.0.2" } }, ["--dev", "BaR", "monkey"])).toBe( + pkgJson({ ...root, devDependencies: { BaR: "^0.0.2", monkey: "^0.0.2" } }), + ); + }); + + it("removes the other group when the package is already in the target group", async () => { + const initial = { ...root, devDependencies: { BaR: "^0.0.2" }, optionalDependencies: { BaR: "^0.0.2" } }; + expect(await add(initial, ["--dev", "BaR"])).toBe(pkgJson({ ...root, devDependencies: { BaR: "^0.0.2" } })); + }); + + it("leaves a peerDependencies entry untouched when adding to devDependencies", async () => { + const initial = { ...root, peerDependencies: { baz: ">=0.0.3" } }; + expect(await add(initial, ["--dev", "baz"], { "0.0.3": {}, "0.0.5": {} })).toBe( + pkgJson({ ...root, peerDependencies: { baz: ">=0.0.3" }, devDependencies: { baz: "^0.0.5" } }), + ); + }); + + it("updates the devDependencies range when adding to peerDependencies", async () => { + const initial = { ...root, devDependencies: { baz: ">=0.0.3" } }; + expect(await add(initial, ["--peer", "baz"], { "0.0.3": {}, "0.0.5": {} })).toBe( + pkgJson({ ...root, devDependencies: { baz: "^0.0.5" }, peerDependencies: { baz: "^0.0.5" } }), + ); + }); + + it("does not move when no group flag is given", async () => { + const initial = { ...root, devDependencies: { BaR: "^0.0.2" } }; + expect(await add(initial, ["BaR"])).toBe(pkgJson(initial)); + }); + + it("does not move with --only-missing", async () => { + const initial = { ...root, dependencies: { BaR: "^0.0.2" } }; + expect(await add(initial, ["--dev", "--only-missing", "BaR"])).toBe(pkgJson(initial)); + }); +}); + it("should add exact version with install.exact", async () => { const urls: string[] = []; setHandler(dummyRegistry(urls)); @@ -1680,7 +1773,7 @@ it("should let you add the same package twice", async () => { { name: "Foo", version: "0.0.1", - dependencies: { + devDependencies: { baz: "^0.0.3", }, },