From 4d004b90cad97debfb4cf1e22a6c6327f300649f Mon Sep 17 00:00:00 2001 From: Dylan Conway <35280289+dylan-conway@users.noreply.github.com> Date: Tue, 4 Feb 2025 23:55:57 -0800 Subject: [PATCH] Fix `bun.lock` formatting of `bin` (#17041) --- src/install/bin.zig | 20 +- src/install/bun.lock.zig | 16 +- src/install/install.zig | 5 +- src/semver/SemverString.zig | 2 +- .../__snapshots__/bun-lock.test.ts.snap | 121 ++++++++++++ test/cli/install/bun-lock.test.ts | 178 +++++++++++++++++- 6 files changed, 319 insertions(+), 23 deletions(-) diff --git a/src/install/bin.zig b/src/install/bin.zig index dddf3a52309542..4da91e8a0ba5cd 100644 --- a/src/install/bin.zig +++ b/src/install/bin.zig @@ -327,7 +327,7 @@ pub const Bin = extern struct { try writer.writeAll("{\n"); indent.* += 1; try writeIndent(writer, indent); - try writer.print("{}: {}\n", .{ + try writer.print("{}: {},\n", .{ this.value.named_file[0].fmtJson(buf, .{}), this.value.named_file[1].fmtJson(buf, .{}), }); @@ -339,23 +339,29 @@ pub const Bin = extern struct { try writer.print("{}", .{this.value.dir.fmtJson(buf, .{})}); }, .map => { - try writer.writeAll("{\n"); + try writer.writeByte('{'); indent.* += 1; const list = this.value.map.get(extern_strings); - var first = true; + var any = false; var i: usize = 0; while (i < list.len) : (i += 2) { - if (!first) { - try writer.writeByte(','); + if (!any) { + any = true; + try writer.writeByte('\n'); } try writeIndent(writer, indent); - first = false; - try writer.print("{}: {}", .{ + try writer.print("{}: {},\n", .{ list[i].value.fmtJson(buf, .{}), list[i + 1].value.fmtJson(buf, .{}), }); } + if (!any) { + try writer.writeByte('}'); + indent.* -= 1; + return; + } + indent.* -= 1; try writeIndent(writer, indent); try writer.writeByte('}'); diff --git a/src/install/bun.lock.zig b/src/install/bun.lock.zig index 83ecb7b1de6ad9..80e7702f774a5c 100644 --- a/src/install/bun.lock.zig +++ b/src/install/bun.lock.zig @@ -510,9 +510,9 @@ pub const Stringifier = struct { const name_and_version, const patch_path = value.*; try writeIndent(writer, indent); try writer.print( - \\"{s}": "{s}", + \\{}: {}, \\ - , .{ name_and_version, patch_path.slice(buf) }); + , .{ bun.fmt.formatJSONStringUTF8(name_and_version, .{}), patch_path.fmtJson(buf, .{}) }); } try decIndent(writer, indent); @@ -534,9 +534,9 @@ pub const Stringifier = struct { const name, const version = value.*; try writeIndent(writer, indent); try writer.print( - \\"{s}": "{s}", + \\{}: {}, \\ - , .{ name.slice(buf), version.literal.slice(buf) }); + , .{ name.fmtJson(buf, .{}), version.literal.fmtJson(buf, .{}) }); } try decIndent(writer, indent); @@ -940,9 +940,7 @@ pub const Stringifier = struct { } else { any = true; } - try writer.writeAll( - \\ "os": - ); + try writer.writeAll(" \"os\": "); try Negatable(Npm.OperatingSystem).toJson(meta.os, writer); } @@ -952,9 +950,7 @@ pub const Stringifier = struct { } else { any = true; } - try writer.writeAll( - \\ "cpu": - ); + try writer.writeAll(" \"cpu\": "); try Negatable(Npm.Architecture).toJson(meta.arch, writer); } diff --git a/src/install/install.zig b/src/install/install.zig index d08bbfbf868be0..9001b581c945b3 100644 --- a/src/install/install.zig +++ b/src/install/install.zig @@ -15014,10 +15014,7 @@ pub const PackageManager = struct { if (manager.options.do.summary) { // TODO(dylan-conway): packages aren't installed but we can still print // added/removed/updated direct dependencies. - Output.pretty( - \\ - \\Saved {s} ({d} package{s}) - , .{ + Output.pretty("\nSaved {s} ({d} package{s}) ", .{ switch (save_format) { .text => "bun.lock", .binary => "bun.lockb", diff --git a/src/semver/SemverString.zig b/src/semver/SemverString.zig index 412622d0513e67..04d3f75dc80212 100644 --- a/src/semver/SemverString.zig +++ b/src/semver/SemverString.zig @@ -139,7 +139,7 @@ pub const String = extern struct { } }; - /// Escapes for json. Expects string to be prequoted + /// Escapes for json. Defaults to quoting the string. pub inline fn fmtJson(self: *const String, buf: []const u8, opts: JsonFormatter.Options) JsonFormatter { return .{ .buf = buf, diff --git a/test/cli/install/__snapshots__/bun-lock.test.ts.snap b/test/cli/install/__snapshots__/bun-lock.test.ts.snap index b8c6d8e01c9abc..2523c5077033ac 100644 --- a/test/cli/install/__snapshots__/bun-lock.test.ts.snap +++ b/test/cli/install/__snapshots__/bun-lock.test.ts.snap @@ -283,3 +283,124 @@ exports[`should not deduplicate bundled packages with un-bundled packages 1`] = "3 packages installed", ] `; + +exports[`should not change formatting unexpectedly 1`] = ` +[ + "preinstall", + "", + "+ optional-native@1.0.0", + "+ optional-peer-deps@1.0.0 (v1.0.1 available)", + "+ uses-what-bin@1.0.0 (v1.5.0 available)", + "", + "13 packages installed", +] +`; + +exports[`should not change formatting unexpectedly 2`] = ` +"{ + "lockfileVersion": 1, + "workspaces": { + "": { + "name": "pkg-root", + "dependencies": { + "uses-what-bin": "1.0.0", + }, + "devDependencies": { + "optional-peer-deps": "1.0.0", + }, + "optionalDependencies": { + "optional-native": "1.0.0", + }, + }, + "packages/pkg1": { + "name": "pkg1", + "version": "2.2.2", + "bin": { + "pkg1-1": "bin-1.js", + "pkg1-2": "bin-2.js", + "pkg1-3": "bin-3.js", + }, + "dependencies": { + "bundled-1": "1.0.0", + }, + "peerDependencies": { + "a-dep": "1.0.1", + }, + "optionalPeers": [ + "a-dep", + ], + }, + "packages/pkg2": { + "name": "pkg2", + "bin": { + "pkg2-1": "bin-1.js", + }, + "dependencies": { + "map-bin": "1.0.2", + }, + }, + "packages/pkg3": { + "name": "pkg3", + "binDir": "bin", + "devDependencies": { + "hoist-lockfile-1": "1.0.0", + }, + }, + }, + "trustedDependencies": [ + "uses-what-bin", + ], + "patchedDependencies": { + "optional-peer-deps@1.0.0": "patches/optional-peer-deps@1.0.0.patch", + }, + "overrides": { + "hoist-lockfile-shared": "1.0.1", + }, + "packages": { + "bundled-1": ["bundled-1@1.0.0", "http://localhost:1234/bundled-1/-/bundled-1-1.0.0.tgz", { "dependencies": { "no-deps": "1.0.0" } }, "sha512-YQ/maWZliKQyp1VIdYnPBH6qBHLCQ8Iy6G5vRZFXUHVXufiXT5aTjPVnLQ7xpVAgURFrzd/Fu1113ROLlaJBkQ=="], + + "hoist-lockfile-1": ["hoist-lockfile-1@1.0.0", "http://localhost:1234/hoist-lockfile-1/-/hoist-lockfile-1-1.0.0.tgz", { "dependencies": { "hoist-lockfile-shared": "*" } }, "sha512-E2nwR7egMFDoYjeRno7CAa59kiwkLGfhTFy2Q335JWp2r2bDkwoAt1LdChd5PdGYkbo7SfViHkW44ga+WXA+eA=="], + + "hoist-lockfile-shared": ["hoist-lockfile-shared@1.0.1", "http://localhost:1234/hoist-lockfile-shared/-/hoist-lockfile-shared-1.0.1.tgz", {}, "sha512-wPw8pTRj2OeZ/n7NeixjaSeI7FoM9DbMHWzdLv1kuBesSXJn+17UA0N7LV7t9dREnIMLw7ycRomhDL+56NRBmQ=="], + + "map-bin": ["map-bin@1.0.2", "http://localhost:1234/map-bin/-/map-bin-1.0.2.tgz", { "bin": { "map-bin": "bin/map-bin", "map_bin": "bin/map-bin" } }, "sha512-d5+2d5socrCT/99w16Gdl/wQu+C3WHflIE/3idFuJOW9xuvt+8VW4bDND/kptCNI63w/ePSJoYm327Sn+I7QCQ=="], + + "native-bar-x64": ["native-bar-x64@1.0.0", "http://localhost:1234/native-bar-x64/-/native-bar-x64-1.0.0.tgz", { "os": "none", "cpu": "x64" }, "sha512-wNjF++hO2mWgeg1uyFzyTUq1tWiO/1kEjKqvgf344NmKJ3JiUp58dSaes4b26AoUT/rrrHEU9SGUu550E9/sUA=="], + + "native-foo-x64": ["native-foo-x64@1.0.0", "http://localhost:1234/native-foo-x64/-/native-foo-x64-1.0.0.tgz", { "os": "none", "cpu": "x64" }, "sha512-+KlZNC/c4RF1wx4ZYdrr2ZfouSHMWM4YLT/yCfh97dlIW1JuRs9LnbdUwrsM007hVF0khUGM9TSVcx+elB6NpQ=="], + + "native-foo-x86": ["native-foo-x86@1.0.0", "http://localhost:1234/native-foo-x86/-/native-foo-x86-1.0.0.tgz", { "os": "none", "cpu": "none" }, "sha512-pUktFGar8JctgQh4Ow5Y9bMp3PB5bHBgbC6M3igED5q99z51WErG2GO3LnPG651SyHtRf+zdeMdhGFWzP54apQ=="], + + "native-libc-glibc": ["native-libc-glibc@1.0.0", "http://localhost:1234/native-libc-glibc/-/native-libc-glibc-1.0.0.tgz", {}, "sha512-D7ivPUqV+bs4jZCFt/fm0BRchhE1kO3XMKZ7/Tt3cF2gfJcewMy/zuId79iaVn9aztJYkOk1GWFpMPXmX5rJHA=="], + + "native-libc-musl": ["native-libc-musl@1.0.0", "http://localhost:1234/native-libc-musl/-/native-libc-musl-1.0.0.tgz", {}, "sha512-1uffg8IA4EJ4VUnuZU4zyRO9EyduuNfbqg2MMVCWSMAsQkfzZnNR0hqtL0GW/EuhE8FWU/FE//Srf1px1pnN2Q=="], + + "optional-native": ["optional-native@1.0.0", "http://localhost:1234/optional-native/-/optional-native-1.0.0.tgz", { "optionalDependencies": { "native-bar-x64": "1.0.0", "native-foo-x64": "1.0.0", "native-foo-x86": "1.0.0", "native-libc-glibc": "1.0.0", "native-libc-musl": "1.0.0" } }, "sha512-E+XTkTpxRqU09BnKGkOkS9vk0sPDhPtArBw6FfL5ciYkb7k6EljnqXEQ1b9l0S1YCVZxZkOZIJCYZfCwj7AgSw=="], + + "optional-peer-deps": ["optional-peer-deps@1.0.0", "http://localhost:1234/optional-peer-deps/-/optional-peer-deps-1.0.0.tgz", { "peerDependencies": { "no-deps": "*" }, "optionalPeers": ["no-deps"] }, "sha512-gJZ2WKSXFwQHjjYNxAjYYIwtgNvDnL+CKURXTtOKNDX27XZN0a9bt+cDgLcCVBTy0V/nQ8h6yW7a6fO34Lv22w=="], + + "pkg1": ["pkg1@workspace:packages/pkg1"], + + "pkg2": ["pkg2@workspace:packages/pkg2"], + + "pkg3": ["pkg3@workspace:packages/pkg3"], + + "uses-what-bin": ["uses-what-bin@1.0.0", "http://localhost:1234/uses-what-bin/-/uses-what-bin-1.0.0.tgz", { "dependencies": { "what-bin": "1.0.0" } }, "sha512-87/Emb1Hh7HtsMMU1yXXhI/+/5opQFbnqtR0Yq/1rgr7jp4mzkMU8wQBiYtS8C45GJY6YfdIqq1Dci+0ivJB2g=="], + + "what-bin": ["what-bin@1.0.0", "http://localhost:1234/what-bin/-/what-bin-1.0.0.tgz", { "bin": { "what-bin": "what-bin.js" } }, "sha512-sa99On1k5aDqCvpni/TQ6rLzYprUWBlb8fNwWOzbjDlM24fRr7FKDOuaBO/Y9WEIcZuzoPkCW5EkBCpflj8REQ=="], + + "bundled-1/no-deps": ["no-deps@1.0.0", "http://localhost:1234/no-deps/-/no-deps-1.0.0.tgz", { "bundled": true }, "sha512-v4w12JRjUGvfHDUP8vFDwu0gUWu04j0cv9hLb1Abf9VdaXu4XcrddYFTMVBVvmldKViGWH7jrb6xPJRF0wq6gw=="], + } +} +" +`; + +exports[`should not change formatting unexpectedly 3`] = ` +[ + "preinstall", + "", + "+ bundled-1@1.0.0", + "", + "13 packages installed", +] +`; diff --git a/test/cli/install/bun-lock.test.ts b/test/cli/install/bun-lock.test.ts index 764375bcb995b9..b636435ae97472 100644 --- a/test/cli/install/bun-lock.test.ts +++ b/test/cli/install/bun-lock.test.ts @@ -1,9 +1,13 @@ import { spawn, write, file } from "bun"; import { expect, it, beforeAll, afterAll } from "bun:test"; import { access, copyFile, open, writeFile, exists, cp, rm } from "fs/promises"; -import { bunExe, bunEnv as env, isWindows, VerdaccioRegistry, runBunInstall } from "harness"; +import { bunExe, bunEnv as env, isWindows, VerdaccioRegistry, runBunInstall, toBeValidBin } from "harness"; import { join } from "path"; +expect.extend({ + toBeValidBin, +}); + var registry = new VerdaccioRegistry(); beforeAll(async () => { @@ -273,3 +277,175 @@ it("should not deduplicate bundled packages with un-bundled packages", async () .slice(1); expect(out2).toEqual(out1); }); + +it("should not change formatting unexpectedly", async () => { + const { packageDir, packageJson } = await registry.createTestDir(); + + const patch = `diff --git a/package.json b/package.json +index d156130662798530e852e1afaec5b1c03d429cdc..b4ddf35975a952fdaed99f2b14236519694f850d 100644 +--- a/package.json ++++ b/package.json +@@ -1,6 +1,7 @@ + { + "name": "optional-peer-deps", + "version": "1.0.0", ++ "hi": true, + "peerDependencies": { + "no-deps": "*" + }, +`; + + // attempt to snapshot most things that can be printed + await Promise.all([ + write( + packageJson, + JSON.stringify({ + name: "pkg-root", + version: "1.0.0", + workspaces: ["packages/*"], + scripts: { + preinstall: "echo 'preinstall'", + }, + overrides: { + "hoist-lockfile-shared": "1.0.1", + }, + bin: "index.js", + optionalDependencies: { + "optional-native": "1.0.0", + }, + devDependencies: { + "optional-peer-deps": "1.0.0", + }, + dependencies: { + "uses-what-bin": "1.0.0", + }, + trustedDependencies: ["uses-what-bin"], + patchedDependencies: { + "optional-peer-deps@1.0.0": "patches/optional-peer-deps@1.0.0.patch", + }, + }), + ), + write(join(packageDir, "patches", "optional-peer-deps@1.0.0.patch"), patch), + write(join(packageDir, "index.js"), "console.log('hello world')"), + write( + join(packageDir, "packages", "pkg1", "package.json"), + JSON.stringify({ + name: "pkg1", + version: "2.2.2", + peerDependenciesMeta: { + "a-dep": { + optional: true, + }, + }, + peerDependencies: { + "a-dep": "1.0.1", + }, + dependencies: { + "bundled-1": "1.0.0", + }, + bin: { + "pkg1-1": "bin-1.js", + "pkg1-2": "bin-2.js", + "pkg1-3": "bin-3.js", + }, + scripts: { + install: "echo 'install'", + postinstall: "echo 'postinstall'", + }, + }), + ), + write(join(packageDir, "packages", "pkg1", "bin-1.js"), "console.log('bin-1')"), + write(join(packageDir, "packages", "pkg1", "bin-2.js"), "console.log('bin-2')"), + write(join(packageDir, "packages", "pkg1", "bin-3.js"), "console.log('bin-3')"), + write( + join(packageDir, "packages", "pkg2", "package.json"), + JSON.stringify({ + name: "pkg2", + bin: { + "pkg2-1": "bin-1.js", + }, + dependencies: { + "map-bin": "1.0.2", + }, + }), + ), + write(join(packageDir, "packages", "pkg2", "bin-1.js"), "console.log('bin-1')"), + write( + join(packageDir, "packages", "pkg3", "package.json"), + JSON.stringify({ + name: "pkg3", + directories: { + bin: "bin", + }, + devDependencies: { + "hoist-lockfile-1": "1.0.0", + }, + }), + ), + write(join(packageDir, "packages", "pkg3", "bin", "bin-1.js"), "console.log('bin-1')"), + ]); + + async function checkInstall() { + expect( + await Promise.all([ + exists(join(packageDir, "node_modules", "pkg1", "package.json")), + exists(join(packageDir, "node_modules", "pkg2", "package.json")), + exists(join(packageDir, "node_modules", "pkg3", "package.json")), + file(join(packageDir, "node_modules", "hoist-lockfile-shared", "package.json")).json(), + exists(join(packageDir, "node_modules", "uses-what-bin", "what-bin.txt")), + file(join(packageDir, "node_modules", "optional-peer-deps", "package.json")).json(), + ]), + ).toMatchObject([true, true, true, { name: "hoist-lockfile-shared", version: "1.0.1" }, true, { hi: true }]); + expect(join(packageDir, "node_modules", ".bin", "bin-1.js")).toBeValidBin(join("..", "pkg3", "bin", "bin-1.js")); + expect(join(packageDir, "node_modules", ".bin", "map-bin")).toBeValidBin(join("..", "map-bin", "bin", "map-bin")); + expect(join(packageDir, "node_modules", ".bin", "map_bin")).toBeValidBin(join("..", "map-bin", "bin", "map-bin")); + expect(join(packageDir, "node_modules", ".bin", "pkg1-1")).toBeValidBin(join("..", "pkg1", "bin-1.js")); + expect(join(packageDir, "node_modules", ".bin", "pkg1-2")).toBeValidBin(join("..", "pkg1", "bin-2.js")); + expect(join(packageDir, "node_modules", ".bin", "pkg1-3")).toBeValidBin(join("..", "pkg1", "bin-3.js")); + expect(join(packageDir, "node_modules", ".bin", "pkg2-1")).toBeValidBin(join("..", "pkg2", "bin-1.js")); + expect(join(packageDir, "node_modules", ".bin", "what-bin")).toBeValidBin(join("..", "what-bin", "what-bin.js")); + } + + let { exited, stdout } = spawn({ + cmd: [bunExe(), "install"], + cwd: packageDir, + env, + stdout: "pipe", + stderr: "inherit", + }); + + expect(await exited).toBe(0); + const out1 = (await Bun.readableStreamToText(stdout)) + .replaceAll(/\s*\[[0-9\.]+m?s\]\s*$/g, "") + .split(/\r?\n/) + .slice(1); + expect(out1).toMatchSnapshot(); + + await checkInstall(); + + const lockfile = (await file(join(packageDir, "bun.lock")).text()).replaceAll(/localhost:\d+/g, "localhost:1234"); + expect(lockfile).toMatchSnapshot(); + + await rm(join(packageDir, "node_modules"), { recursive: true, force: true }); + + ({ exited, stdout } = spawn({ + cmd: [bunExe(), "install"], + cwd: join(packageDir, "packages", "pkg1"), + env, + stdout: "pipe", + stderr: "inherit", + })); + + expect(await exited).toBe(0); + const out2 = (await Bun.readableStreamToText(stdout)) + .replaceAll(/\s*\[[0-9\.]+m?s\]\s*$/g, "") + .split(/\r?\n/) + .slice(1); + expect(out2).toMatchSnapshot(); + + await checkInstall(); + + expect((await file(join(packageDir, "bun.lock")).text()).replaceAll(/localhost:\d+/g, "localhost:1234")).toBe( + lockfile, + ); +});