From b0a5728b37e194982b0bfc00325c3cf20b5d15b5 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Tue, 15 Jul 2025 22:14:27 -0700 Subject: [PATCH] Allow `"catalog"` and `"catalogs"` in top-level package.json if it wasn't provided in `"workspaces"` object (#21009) --- docs/install/catalogs.md | 2 + src/install/lockfile/CatalogMap.zig | 7 ++- src/install/lockfile/Package.zig | 13 +++++- test/cli/install/catalogs.test.ts | 72 +++++++++++++++++------------ 4 files changed, 63 insertions(+), 31 deletions(-) diff --git a/docs/install/catalogs.md b/docs/install/catalogs.md index aeec6d691a..bbf0dbf72a 100644 --- a/docs/install/catalogs.md +++ b/docs/install/catalogs.md @@ -52,6 +52,8 @@ In your root-level `package.json`, add a `catalog` or `catalogs` field within th } ``` +If you put `catalog` or `catalogs` at the top level of the `package.json` file, that will work too. + ### 2. Reference Catalog Versions in Workspace Packages In your workspace packages, use the `catalog:` protocol to reference versions: diff --git a/src/install/lockfile/CatalogMap.zig b/src/install/lockfile/CatalogMap.zig index a531ffc24e..6e0cac7980 100644 --- a/src/install/lockfile/CatalogMap.zig +++ b/src/install/lockfile/CatalogMap.zig @@ -113,9 +113,11 @@ pub fn parseAppend( source: *const logger.Source, expr: Expr, builder: *Lockfile.StringBuilder, -) OOM!void { +) OOM!bool { + var found_any = false; if (expr.get("catalog")) |default_catalog| { const group = try this.getOrPutGroup(lockfile, .empty); + found_any = true; switch (default_catalog.data) { .e_object => |obj| { for (obj.properties.slice()) |item| { @@ -171,6 +173,7 @@ pub fn parseAppend( } if (expr.get("catalogs")) |catalogs| { + found_any = true; switch (catalogs.data) { .e_object => |catalog_names| { for (catalog_names.properties.slice()) |catalog| { @@ -234,6 +237,8 @@ pub fn parseAppend( else => {}, } } + + return found_any; } pub fn sort(this: *CatalogMap, lockfile: *const Lockfile) void { diff --git a/src/install/lockfile/Package.zig b/src/install/lockfile/Package.zig index 8edae3a083..6e3350b1cf 100644 --- a/src/install/lockfile/Package.zig +++ b/src/install/lockfile/Package.zig @@ -1958,8 +1958,19 @@ pub const Package = extern struct { // This function depends on package.dependencies being set, so it is done at the very end. if (comptime features.is_main) { try lockfile.overrides.parseAppend(pm, lockfile, package, log, source, json, &string_builder); + var found_any_catalog_or_catalog_object = false; + var has_workspaces = false; if (json.get("workspaces")) |workspaces_expr| { - try lockfile.catalogs.parseAppend(pm, lockfile, log, source, workspaces_expr, &string_builder); + found_any_catalog_or_catalog_object = try lockfile.catalogs.parseAppend(pm, lockfile, log, source, workspaces_expr, &string_builder); + has_workspaces = true; + } + + // `"workspaces"` being an object instead of an array is sometimes + // unexpected to people. therefore if you also are using workspaces, + // allow "catalog" and "catalogs" in top-level "package.json" + // so it's easier to guess. + if (!found_any_catalog_or_catalog_object and has_workspaces) { + _ = try lockfile.catalogs.parseAppend(pm, lockfile, log, source, json, &string_builder); } } diff --git a/test/cli/install/catalogs.test.ts b/test/cli/install/catalogs.test.ts index c80b19e566..831cdc67b9 100644 --- a/test/cli/install/catalogs.test.ts +++ b/test/cli/install/catalogs.test.ts @@ -15,21 +15,32 @@ afterAll(() => { }); describe("basic", () => { - async function createBasicCatalogMonorepo(packageDir: string, name: string) { - const packageJson = { - name, - workspaces: { - packages: ["packages/*"], - catalog: { - "no-deps": "2.0.0", - }, - catalogs: { - a: { - "a-dep": "1.0.1", - }, + async function createBasicCatalogMonorepo(packageDir: string, name: string, inTopLevelKey: boolean = false) { + const catalogs = { + catalog: { + "no-deps": "2.0.0", + }, + catalogs: { + a: { + "a-dep": "1.0.1", }, }, }; + const packageJson = !inTopLevelKey + ? { + name, + workspaces: { + packages: ["packages/*"], + ...catalogs, + }, + } + : { + name, + ...catalogs, + workspaces: { + packages: ["packages/*"], + }, + }; await Promise.all([ write(join(packageDir, "package.json"), JSON.stringify(packageJson)), @@ -47,26 +58,29 @@ describe("basic", () => { return packageJson; } - test("both catalog and catalogs", async () => { - const { packageDir } = await registry.createTestDir(); - await createBasicCatalogMonorepo(packageDir, "catalog-basic-1"); + for (const isTopLevel of [true, false]) { + test(`both catalog and catalogs ${isTopLevel ? "in top-level" : "in workspaces"}`, async () => { + const { packageDir } = await registry.createTestDir(); - await runBunInstall(bunEnv, packageDir); + await createBasicCatalogMonorepo(packageDir, "catalog-basic-1", isTopLevel); - expect(await file(join(packageDir, "node_modules", "no-deps", "package.json")).json()).toEqual({ - name: "no-deps", - version: "2.0.0", + await runBunInstall(bunEnv, packageDir); + + expect(await file(join(packageDir, "node_modules", "no-deps", "package.json")).json()).toEqual({ + name: "no-deps", + version: "2.0.0", + }); + + expect(await file(join(packageDir, "node_modules", "a-dep", "package.json")).json()).toEqual({ + name: "a-dep", + version: "1.0.1", + }); + + // another install does not save the lockfile + await runBunInstall(bunEnv, packageDir, { savesLockfile: false }); }); - - expect(await file(join(packageDir, "node_modules", "a-dep", "package.json")).json()).toEqual({ - name: "a-dep", - version: "1.0.1", - }); - - // another install does not save the lockfile - await runBunInstall(bunEnv, packageDir, { savesLockfile: false }); - }); + } for (const binaryLockfile of [true, false]) { test(`detect changes (${binaryLockfile ? "bun.lockb" : "bun.lock"})`, async () => { @@ -122,7 +136,7 @@ describe("basic", () => { }); // update catalogs - packageJson.workspaces.catalogs.a["a-dep"] = "1.0.10"; + packageJson.workspaces!.catalogs!.a["a-dep"] = "1.0.10"; await write(join(packageDir, "package.json"), JSON.stringify(packageJson)); ({ err } = await runBunInstall(bunEnv, packageDir, { savesLockfile: true })); expect(err).toContain("Saved lockfile");