diff --git a/src/install/PackageManager/CommandLineArguments.zig b/src/install/PackageManager/CommandLineArguments.zig index b184207f50..0d20418e92 100644 --- a/src/install/PackageManager/CommandLineArguments.zig +++ b/src/install/PackageManager/CommandLineArguments.zig @@ -449,6 +449,10 @@ pub fn printHelp(subcommand: Subcommand) void { \\ bun add --optional lodash \\ bun add --peer esbuild \\ + \\ Add with catalog reference (monorepo version management) + \\ bun add --catalog react + \\ bun add --catalog=dev typescript + \\ \\Full documentation is available at https://bun.com/docs/cli/add. \\ ; diff --git a/src/install/PackageManager/PackageJSONEditor.zig b/src/install/PackageManager/PackageJSONEditor.zig index 37977ea419..12d18a7d80 100644 --- a/src/install/PackageManager/PackageJSONEditor.zig +++ b/src/install/PackageManager/PackageJSONEditor.zig @@ -5,6 +5,8 @@ const dependency_groups = &.{ .{ "peerDependencies", .{ .peer = true } }, }; +const CATALOG_PREFIX = "catalog:"; + pub const EditOptions = struct { exact_versions: bool = false, add_trusted_dependencies: bool = false, @@ -12,6 +14,25 @@ pub const EditOptions = struct { catalog_name: ?string = null, }; +/// Validates and formats a catalog reference string. +/// Returns null if the catalog name is invalid (contains whitespace or ':'). +fn formatCatalogReference(allocator: std.mem.Allocator, catalog_name: ?string) !?string { + const catalog = catalog_name orelse return null; + const trimmed = strings.trim(catalog, " \t\r\n"); + + // Validate: catalog name must not contain whitespace or ':' + for (trimmed) |c| { + if (c == ':' or std.ascii.isWhitespace(c)) { + return null; // Invalid catalog name + } + } + + if (trimmed.len == 0) { + return try allocator.dupe(u8, CATALOG_PREFIX); + } + return try std.fmt.allocPrint(allocator, "{s}{s}", .{ CATALOG_PREFIX, trimmed }); +} + pub fn editCatalog( manager: *PackageManager, package_json: *Expr, @@ -52,7 +73,7 @@ pub fn editCatalog( if (request.package_id < resolutions.len and resolutions[request.package_id].tag == .npm) { const version_str = try std.fmt.allocPrint( allocator, - "^{}", + "^{f}", .{resolutions[request.package_id].value.npm.version.fmt(manager.lockfile.buffers.string_bytes.items)}, ); const version_expr = try Expr.init(E.String, E.String{ .data = version_str }, logger.Loc.Empty).clone(allocator); @@ -85,7 +106,7 @@ pub fn editCatalog( if (request.package_id < resolutions.len and resolutions[request.package_id].tag == .npm) { const version_str = try std.fmt.allocPrint( allocator, - "^{}", + "^{f}", .{resolutions[request.package_id].value.npm.version.fmt(manager.lockfile.buffers.string_bytes.items)}, ); const version_expr = try Expr.init(E.String, E.String{ .data = version_str }, logger.Loc.Empty).clone(allocator); @@ -769,12 +790,15 @@ pub fn edit( for (updates.*) |*request| { if (request.e_string) |e_string| { // If catalog is specified, use catalog reference - if (options.catalog_name) |catalog| { - e_string.data = if (catalog.len == 0) - try allocator.dupe(u8, "catalog:") - else - try std.fmt.allocPrint(allocator, "catalog:{s}", .{catalog}); - continue; + if (options.catalog_name != null) { + if (try formatCatalogReference(allocator, options.catalog_name)) |catalog_ref| { + e_string.data = catalog_ref; + continue; + } else { + // Invalid catalog name - log error and skip catalog reference + Output.errGeneric("Invalid catalog name: contains whitespace or ':'", .{}); + Global.exit(1); + } } if (request.package_id >= resolutions.len or resolutions[request.package_id].tag == .uninitialized) { @@ -883,6 +907,8 @@ const std = @import("std"); const bun = @import("bun"); const Environment = bun.Environment; +const Global = bun.Global; +const Output = bun.Output; const Semver = bun.Semver; const logger = bun.logger; const strings = bun.strings; diff --git a/src/install/PackageManager/updatePackageJSONAndInstall.zig b/src/install/PackageManager/updatePackageJSONAndInstall.zig index 85f1aec34a..9c4763a3d9 100644 --- a/src/install/PackageManager/updatePackageJSONAndInstall.zig +++ b/src/install/PackageManager/updatePackageJSONAndInstall.zig @@ -1,3 +1,18 @@ +/// Helper to write package.json content to disk. +/// Opens the file, writes the content, truncates to the exact length, and closes. +fn writePackageJSONToDisk(path: [:0]const u8, content: []const u8) !void { + const file = (try bun.sys.File.openat( + .cwd(), + path, + bun.O.RDWR, + 0, + ).unwrap()).handle.stdFile(); + + try file.pwriteAll(content, 0); + std.posix.ftruncate(file.handle, content.len) catch {}; + file.close(); +} + pub fn updatePackageJSONAndInstallWithManager( manager: *PackageManager, ctx: Command.Context, @@ -385,16 +400,7 @@ fn updatePackageJSONAndInstallWithManagerWithUpdates( // Write root package.json to disk if (manager.options.do.write_package_json) { - const root_package_json_file = (try bun.sys.File.openat( - .cwd(), - root_package_json_path, - bun.O.RDWR, - 0, - ).unwrap()).handle.stdFile(); - - try root_package_json_file.pwriteAll(root_package_json_source, 0); - std.posix.ftruncate(root_package_json_file.handle, root_package_json_source.len) catch {}; - root_package_json_file.close(); + try writePackageJSONToDisk(root_package_json_path, root_package_json_source); } } @@ -476,16 +482,7 @@ fn updatePackageJSONAndInstallWithManagerWithUpdates( // Now that we've run the install step // We can save our in-memory package.json to disk - const workspace_package_json_file = (try bun.sys.File.openat( - .cwd(), - path, - bun.O.RDWR, - 0, - ).unwrap()).handle.stdFile(); - - try workspace_package_json_file.pwriteAll(source, 0); - std.posix.ftruncate(workspace_package_json_file.handle, source.len) catch {}; - workspace_package_json_file.close(); + try writePackageJSONToDisk(path, source); if (subcommand == .remove) { if (!any_changes) { diff --git a/test/cli/install/bun-add-catalog.test.ts b/test/cli/install/bun-add-catalog.test.ts index 2b0c5668ed..a704bccf90 100644 --- a/test/cli/install/bun-add-catalog.test.ts +++ b/test/cli/install/bun-add-catalog.test.ts @@ -1,34 +1,32 @@ import { file, spawn } from "bun"; -import { afterAll, beforeAll, describe, expect, test } from "bun:test"; -import { mkdirSync, writeFileSync } from "fs"; -import { VerdaccioRegistry, bunEnv, bunExe } from "harness"; +import { afterAll, beforeEach, describe, expect, test } from "bun:test"; +import { existsSync, mkdirSync, writeFileSync } from "fs"; +import { VerdaccioRegistry, bunExe, bunEnv as env } from "harness"; import { join } from "path"; var registry = new VerdaccioRegistry(); +var packageDir: string; -beforeAll(async () => { - await registry.start(); -}); +await registry.start(); afterAll(() => { registry.stop(); }); +beforeEach(async () => { + ({ packageDir } = await registry.createTestDir()); +}); + describe("bun add --catalog", () => { test("bun add --catalog adds dependency with catalog reference and populates catalog", async () => { - const { packageDir } = await registry.createTestDir(); - // Create initial package.json WITHOUT catalog - it should be created - const initialPackageJson = { - name: "test-catalog-add", - }; - writeFileSync(join(packageDir, "package.json"), JSON.stringify(initialPackageJson, null, 2)); + writeFileSync(join(packageDir, "package.json"), JSON.stringify({ name: "test-catalog-add" }, null, 2)); // Run bun add --catalog no-deps await using proc = spawn({ cmd: [bunExe(), "add", "--catalog", "no-deps"], cwd: packageDir, - env: bunEnv, + env, stdout: "pipe", stderr: "pipe", }); @@ -43,6 +41,9 @@ describe("bun add --catalog", () => { // The add command should succeed expect(exitCode).toBe(0); + // Verify node_modules was NOT created (catalog mode shouldn't install) + expect(existsSync(join(packageDir, "node_modules"))).toBe(false); + // Check that package.json was updated with catalog reference const updatedPackageJson = await file(join(packageDir, "package.json")).json(); expect(updatedPackageJson.dependencies).toEqual({ @@ -55,19 +56,14 @@ describe("bun add --catalog", () => { }); test("bun add --catalog=name adds dependency with named catalog reference and populates catalog", async () => { - const { packageDir } = await registry.createTestDir(); - // Create initial package.json WITHOUT named catalog - it should be created - const initialPackageJson = { - name: "test-catalog-add-named", - }; - writeFileSync(join(packageDir, "package.json"), JSON.stringify(initialPackageJson, null, 2)); + writeFileSync(join(packageDir, "package.json"), JSON.stringify({ name: "test-catalog-add-named" }, null, 2)); // Run bun add --catalog=dev a-dep await using proc = spawn({ cmd: [bunExe(), "add", "--catalog=dev", "a-dep"], cwd: packageDir, - env: bunEnv, + env, stdout: "pipe", stderr: "pipe", }); @@ -94,19 +90,14 @@ describe("bun add --catalog", () => { }); test("bun add --catalog with --dev flag", async () => { - const { packageDir } = await registry.createTestDir(); - // Create initial package.json - const initialPackageJson = { - name: "test-catalog-add-dev", - }; - writeFileSync(join(packageDir, "package.json"), JSON.stringify(initialPackageJson, null, 2)); + writeFileSync(join(packageDir, "package.json"), JSON.stringify({ name: "test-catalog-add-dev" }, null, 2)); // Run bun add --catalog --dev no-deps await using proc = spawn({ cmd: [bunExe(), "add", "--catalog", "--dev", "no-deps"], cwd: packageDir, - env: bunEnv, + env, stdout: "pipe", stderr: "pipe", }); @@ -132,28 +123,22 @@ describe("bun add --catalog", () => { }); test("bun add --catalog works in monorepo workspace", async () => { - const { packageDir } = await registry.createTestDir(); - // Create root package.json without catalog - const rootPackageJson = { - name: "monorepo-root", - workspaces: ["packages/*"], - }; - writeFileSync(join(packageDir, "package.json"), JSON.stringify(rootPackageJson, null, 2)); + writeFileSync( + join(packageDir, "package.json"), + JSON.stringify({ name: "monorepo-root", workspaces: ["packages/*"] }, null, 2), + ); // Create workspace package const workspaceDir = join(packageDir, "packages", "pkg1"); mkdirSync(workspaceDir, { recursive: true }); - const workspacePackageJson = { - name: "pkg1", - }; - writeFileSync(join(workspaceDir, "package.json"), JSON.stringify(workspacePackageJson, null, 2)); + writeFileSync(join(workspaceDir, "package.json"), JSON.stringify({ name: "pkg1" }, null, 2)); // Run bun add --catalog from workspace directory await using proc = spawn({ cmd: [bunExe(), "add", "--catalog", "no-deps"], cwd: workspaceDir, - env: bunEnv, + env, stdout: "pipe", stderr: "pipe", }); @@ -180,19 +165,14 @@ describe("bun add --catalog", () => { }); test("bun add --catalog multiple packages", async () => { - const { packageDir } = await registry.createTestDir(); - // Create initial package.json - const initialPackageJson = { - name: "test-catalog-add-multiple", - }; - writeFileSync(join(packageDir, "package.json"), JSON.stringify(initialPackageJson, null, 2)); + writeFileSync(join(packageDir, "package.json"), JSON.stringify({ name: "test-catalog-add-multiple" }, null, 2)); // Run bun add --catalog with multiple packages await using proc = spawn({ cmd: [bunExe(), "add", "--catalog", "no-deps", "a-dep"], cwd: packageDir, - env: bunEnv, + env, stdout: "pipe", stderr: "pipe", }); @@ -218,4 +198,32 @@ describe("bun add --catalog", () => { expect(updatedPackageJson.catalog["no-deps"]).toMatch(/^\^2\.0\.0$/); expect(updatedPackageJson.catalog["a-dep"]).toMatch(/^\^1\.0\.\d+$/); }); + + test("bun add --catalog --no-save does not modify package.json", async () => { + // Create initial package.json + const initialContent = JSON.stringify({ name: "test-no-save" }, null, 2); + writeFileSync(join(packageDir, "package.json"), initialContent); + + // Run bun add --catalog --no-save no-deps + await using proc = spawn({ + cmd: [bunExe(), "add", "--catalog", "--no-save", "no-deps"], + cwd: packageDir, + env, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + if (exitCode !== 0) { + console.log("stdout:", stdout); + console.log("stderr:", stderr); + } + + expect(exitCode).toBe(0); + + // Verify package.json was not modified + const finalContent = await file(join(packageDir, "package.json")).text(); + expect(finalContent).toBe(initialContent); + }); });