From 0baf5b94e2fceffaecc4f533ea41c8cab8556a5e Mon Sep 17 00:00:00 2001 From: "taylor.fish" Date: Sat, 12 Jul 2025 02:10:45 -0700 Subject: [PATCH] Fix `bun add` with multiple packages (#20987) Co-authored-by: Meghan Denny --- .../PackageManager/PackageJSONEditor.zig | 77 +++++-------------- src/install/PackageManager/UpdateRequest.zig | 21 ++++- test/cli/install/bun-add.test.ts | 56 +++++++++++++- 3 files changed, 94 insertions(+), 60 deletions(-) diff --git a/src/install/PackageManager/PackageJSONEditor.zig b/src/install/PackageManager/PackageJSONEditor.zig index d42edc35a0..f65f579226 100644 --- a/src/install/PackageManager/PackageJSONEditor.zig +++ b/src/install/PackageManager/PackageJSONEditor.zig @@ -407,10 +407,7 @@ pub fn edit( inline for ([_]string{ "dependencies", "devDependencies", "optionalDependencies", "peerDependencies" }) |list| { if (current_package_json.asProperty(list)) |query| { if (query.expr.data == .e_object) { - const name = if (request.is_aliased) - request.name - else - request.version.literal.slice(request.version_buf); + const name = request.getName(); if (query.expr.asProperty(name)) |value| { if (value.expr.data == .e_string) { @@ -537,69 +534,37 @@ pub fn edit( break :brk deps; }; - outer: for (updates.*) |*request| { + for (updates.*) |*request| { if (request.e_string != null) continue; defer if (comptime Environment.allow_assert) bun.assert(request.e_string != null); var k: usize = 0; while (k < new_dependencies.len) : (k += 1) { if (new_dependencies[k].key) |key| { - if (!request.is_aliased and request.package_id != invalid_package_id and key.data.e_string.eql( - string, - manager.lockfile.packages.items(.name)[request.package_id].slice(request.version_buf), - )) { - // This actually is a duplicate which we did not - // pick up before dependency resolution. - // For this case, we'll just swap remove it. - if (new_dependencies.len > 1) { - new_dependencies[k] = new_dependencies[new_dependencies.len - 1]; - new_dependencies = new_dependencies[0 .. new_dependencies.len - 1]; - } else { - new_dependencies = &[_]G.Property{}; - } - continue; - } - if (key.data.e_string.eql( - string, - if (request.is_aliased) - request.name - else - request.version.literal.slice(request.version_buf), - )) { - if (request.package_id == invalid_package_id) { - // This actually is a duplicate like "react" - // appearing in both "dependencies" and "optionalDependencies". - // For this case, we'll just swap remove it - if (new_dependencies.len > 1) { - new_dependencies[k] = new_dependencies[new_dependencies.len - 1]; - new_dependencies = new_dependencies[0 .. new_dependencies.len - 1]; - } else { - new_dependencies = &[_]G.Property{}; - } - continue; - } - - new_dependencies[k].key = null; + const name = request.getName(); + if (!key.data.e_string.eql(string, name)) continue; + if (request.package_id == invalid_package_id) { + // Duplicate dependency (e.g., "react" in both "dependencies" and + // "optionalDependencies"). Remove the old dependency. + new_dependencies[k] = .{}; + new_dependencies = new_dependencies[0 .. new_dependencies.len - 1]; } } - if (new_dependencies[k].key == null) { - new_dependencies[k].key = JSAst.Expr.allocate( - allocator, - JSAst.E.String, - .{ .data = try allocator.dupe(u8, request.getResolvedName(manager.lockfile)) }, - logger.Loc.Empty, - ); + new_dependencies[k].key = JSAst.Expr.allocate( + allocator, + JSAst.E.String, + .{ .data = try allocator.dupe(u8, request.getResolvedName(manager.lockfile)) }, + logger.Loc.Empty, + ); - new_dependencies[k].value = JSAst.Expr.allocate(allocator, JSAst.E.String, .{ - // we set it later - .data = "", - }, logger.Loc.Empty); + new_dependencies[k].value = JSAst.Expr.allocate(allocator, JSAst.E.String, .{ + // we set it later + .data = "", + }, logger.Loc.Empty); - request.e_string = new_dependencies[k].value.?.data.e_string; - - if (request.is_aliased) continue :outer; - } + request.e_string = new_dependencies[k].value.?.data.e_string; + break; } } diff --git a/src/install/PackageManager/UpdateRequest.zig b/src/install/PackageManager/UpdateRequest.zig index 2c670d37a1..c6001ff01d 100644 --- a/src/install/PackageManager/UpdateRequest.zig +++ b/src/install/PackageManager/UpdateRequest.zig @@ -17,6 +17,21 @@ pub inline fn matches(this: PackageManager.UpdateRequest, dependency: Dependency dependency.name_hash; } +pub fn getName(this: *const UpdateRequest) string { + return if (this.is_aliased) + this.name + else + this.version.literal.slice(this.version_buf); +} + +/// If `this.package_id` is not `invalid_package_id`, it must be less than `lockfile.packages.len`. +pub fn getNameInLockfile(this: *const UpdateRequest, lockfile: *const Lockfile) ?string { + return if (this.package_id == invalid_package_id) + null + else + lockfile.packages.items(.name)[this.package_id].slice(this.version_buf); +} + /// It is incorrect to call this function before Lockfile.cleanWithLogger() because /// resolved_name should be populated if possible. /// @@ -25,10 +40,10 @@ pub inline fn matches(this: PackageManager.UpdateRequest, dependency: Dependency pub fn getResolvedName(this: *const UpdateRequest, lockfile: *const Lockfile) string { return if (this.is_aliased) this.name - else if (this.package_id == invalid_package_id) - this.version.literal.slice(this.version_buf) + else if (this.getNameInLockfile(lockfile)) |name| + name else - lockfile.packages.items(.name)[this.package_id].slice(this.version_buf); + this.version.literal.slice(this.version_buf); } pub fn fromJS(globalThis: *JSC.JSGlobalObject, input: JSC.JSValue) bun.JSError!JSC.JSValue { diff --git a/test/cli/install/bun-add.test.ts b/test/cli/install/bun-add.test.ts index b8455aec02..9c53629575 100644 --- a/test/cli/install/bun-add.test.ts +++ b/test/cli/install/bun-add.test.ts @@ -187,6 +187,7 @@ it("bun add --only-missing should not install existing package", async () => { stdout: "pipe", stdin: "pipe", stderr: "pipe", + env, }); const out = await new Response(stdout).text(); expect(out).not.toContain("Saved lockfile"); @@ -260,6 +261,7 @@ it("bun add --analyze should scan dependencies", async () => { stdout: "pipe", stdin: "pipe", stderr: "pipe", + env, }); const out = await new Response(stdout).text(); expect(out).not.toContain("Saved lockfile"); @@ -601,7 +603,7 @@ it("should add to devDependencies with --dev", async () => { ); await access(join(package_dir, "bun.lockb")); }); -it.only("should add to optionalDependencies with --optional", async () => { +it("should add to optionalDependencies with --optional", async () => { const urls: string[] = []; setHandler(dummyRegistry(urls)); await writeFile( @@ -818,6 +820,7 @@ it("should add exact version with -E", async () => { }); it("should add dependency with package.json in it and http tarball", async () => { + const old_check_npm_auth_type = check_npm_auth_type.check; check_npm_auth_type.check = false; using server = Bun.serve({ port: 0, @@ -902,6 +905,8 @@ it("should add dependency with package.json in it and http tarball", async () => }, }); await access(join(package_dir, "bun.lockb")); + // Reset to old value for other tests + check_npm_auth_type.check = old_check_npm_auth_type; }); it("should add dependency with specified semver", async () => { @@ -2269,3 +2274,52 @@ it("should add local tarball dependency", async () => { (expect(await file(join(package_dir, "package.json")).text()).toInclude('"baz-0.0.3.tgz"'), await access(join(package_dir, "bun.lockb"))); }); + +it("should add multiple dependencies specified on command line", async () => { + expect(check_npm_auth_type.check).toBe(true); + using server = Bun.serve({ + port: 0, + fetch(req) { + return new Response(Bun.file(join(__dirname, "baz-0.0.3.tgz"))); + }, + }); + const server_url = server.url.href.replace(/\/+$/, ""); + const urls: string[] = []; + setHandler(dummyRegistry(urls)); + const { stdout, stderr, exited } = spawn({ + cmd: [bunExe(), "add", `${server_url}/baz-0.0.3.tgz`, "bar"], + cwd: package_dir, + stdout: "pipe", + stdin: "pipe", + stderr: "pipe", + env, + }); + await writeFile( + join(add_dir, "package.json"), + JSON.stringify({ + name: "foo", + version: "0.0.1", + }), + ); + const err = await new Response(stderr).text(); + expect(err).not.toContain("error:"); + expect(err).toContain("Saved lockfile"); + const out = await new Response(stdout).text(); + expect(out.replace(/\s*\[[0-9\.]+m?s\]\s*$/, "").split(/\r?\n/)).toEqual([ + expect.stringMatching(/^bun add v1./), + "", + expect.stringMatching(/^installed baz@http:\/\/.* with binaries:$/), + " - baz-run", + "installed bar@0.0.2", + "", + "2 packages installed", + ]); + expect(await exited).toBe(0); + expect(await file(join(package_dir, "package.json")).json()).toStrictEqual({ + dependencies: { + bar: "^0.0.2", + baz: `${server_url}/baz-0.0.3.tgz`, + }, + }); + await access(join(package_dir, "bun.lockb")); +});