Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 55 additions & 2 deletions src/install/PackageManager/PackageJSONEditor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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(_)) {
Expand All @@ -1004,14 +1008,48 @@ pub(crate) fn edit(
== dependency::Tag::Catalog
},
);
let in_target_list =
strings::eql_long(list, dependency_list, true);

// `bun update <name>` 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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}

Expand Down
13 changes: 11 additions & 2 deletions test/cli/install/bun-add-catalog.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down
9 changes: 4 additions & 5 deletions test/cli/install/bun-add-filter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
Expand Down
95 changes: 94 additions & 1 deletion test/cli/install/bun-add.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, unknown>) => JSON.stringify(pkg, null, 2);

async function add(
initial: Record<string, unknown>,
args: string[],
registry: Record<string, unknown> = { "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));
Expand Down Expand Up @@ -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",
},
},
Expand Down