mirrors/bun.sh
1
0
Fork 0
mirror of https://github.com/oven-sh/bun synced 2026-02-09 10:28:47 +00:00
Code Issues Packages Projects Releases Wiki Activity

Address PR review comments for --catalog flag

Browse Source 
Changes:
- Add --catalog usage examples to add subcommand help text
- Add input validation for catalog name (trim whitespace, reject ':' and whitespace chars)
- Extract writePackageJSONToDisk helper to reduce code duplication
- Update tests to use beforeEach pattern and add --no-save test
- Fix format string in editCatalog to use {f} for version formatter

The prefetch disabling suggestion was not implemented because --catalog
needs to run installation to resolve package versions before writing
catalog references.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Claude Bot
2025-12-16 03:27:59 +00:00
parent 1c329161c9
commit e06141bbdc
4 changed files with 109 additions and 74 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
src/install/PackageManager/CommandLineArguments.zig
View File

@@ -449,6 +449,10 @@ pub fn printHelp(subcommand: Subcommand) void {
\\ <b><green>bun add<r> <cyan>--optional<r> <blue>lodash<r>
\\ <b><green>bun add<r> <cyan>--peer<r> <blue>esbuild<r>
\\
\\ <d>Add with catalog reference (monorepo version management)<r>
\\ <b><green>bun add<r> <cyan>--catalog<r> <blue>react<r>
\\ <b><green>bun add<r> <cyan>--catalog=dev<r> <blue>typescript<r>
\\
\\Full documentation is available at <magenta>https://bun.com/docs/cli/add<r>.
\\
;

42
src/install/PackageManager/PackageJSONEditor.zig
View File

@@ -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;

37
src/install/PackageManager/updatePackageJSONAndInstall.zig
View File

@@ -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) {

100
test/cli/install/bun-add-catalog.test.ts
View File

@@ -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);
});
});