From 4844c29cc4169d612ea88bb5b0cce4baa2dd0ab5 Mon Sep 17 00:00:00 2001 From: Zack Radisic <56137411+zackradisic@users.noreply.github.com> Date: Fri, 21 Jun 2024 14:16:14 -0700 Subject: [PATCH] Fix `bun patch` with workspaces and scoped packages (#12022) Co-authored-by: Jarred Sumner Co-authored-by: Dylan Conway <35280289+dylan-conway@users.noreply.github.com> --- src/install/install.zig | 114 +++++++++++++++++----- src/string_immutable.zig | 9 +- test/cli/install/bun-patch.test.ts | 150 +++++++++++++++++++++++++++++ 3 files changed, 249 insertions(+), 24 deletions(-) diff --git a/src/install/install.zig b/src/install/install.zig index 08f251a185f30a..d4006da4be1d85 100644 --- a/src/install/install.zig +++ b/src/install/install.zig @@ -10051,6 +10051,7 @@ pub const PackageManager = struct { "dependencies"; var any_changes = false; + var not_in_workspace_root: ?PatchCommitResult = null; switch (subcommand) { .remove => { // if we're removing, they don't have to specify where it is installed in the dependencies list @@ -10129,12 +10130,18 @@ pub const PackageManager = struct { if (manager.options.patch_features == .commit) { var pathbuf: bun.PathBuffer = undefined; if (try manager.doPatchCommit(&pathbuf, log_level)) |stuff| { - try PackageJSONEditor.editPatchedDependencies( - manager, - ¤t_package_json.root, - stuff.patch_key, - stuff.patchfile_path, - ); + // we're inside a workspace package, we need to edit the + // root json, not the `current_package_json` + if (stuff.not_in_workspace_root) { + not_in_workspace_root = stuff; + } else { + try PackageJSONEditor.editPatchedDependencies( + manager, + ¤t_package_json.root, + stuff.patch_key, + stuff.patchfile_path, + ); + } } } }, @@ -10172,6 +10179,8 @@ pub const PackageManager = struct { @memcpy(root_package_json_path_buf[0..top_level_dir_without_trailing_slash.len], top_level_dir_without_trailing_slash); @memcpy(root_package_json_path_buf[top_level_dir_without_trailing_slash.len..][0.."/package.json".len], "/package.json"); const root_package_json_path = root_package_json_path_buf[0 .. top_level_dir_without_trailing_slash.len + "/package.json".len]; + root_package_json_path_buf[root_package_json_path.len] = 0; + const root_package_json_pathZ = root_package_json_path_buf[0..root_package_json_path.len :0]; const root_package_json = switch (manager.workspace_package_json_cache.getWithPath(manager.allocator, manager.log, root_package_json_path, .{})) { .parse_err => |err| { @@ -10196,6 +10205,25 @@ pub const PackageManager = struct { .entry => |entry| entry, }; + if (not_in_workspace_root) |stuff| { + try PackageJSONEditor.editPatchedDependencies( + manager, + &root_package_json.root, + stuff.patch_key, + stuff.patchfile_path, + ); + var buffer_writer2 = try JSPrinter.BufferWriter.init(manager.allocator); + try buffer_writer2.buffer.list.ensureTotalCapacity(manager.allocator, root_package_json.source.contents.len + 1); + buffer_writer2.append_newline = preserve_trailing_newline_at_eof_for_package_json; + var package_json_writer2 = JSPrinter.BufferPrinter.init(buffer_writer2); + + _ = JSPrinter.printJSON(@TypeOf(&package_json_writer2), &package_json_writer2, root_package_json.root, &root_package_json.source) catch |err| { + Output.prettyErrorln("package.json failed to write due to error {s}", .{@errorName(err)}); + Global.crash(); + }; + root_package_json.source.contents = try manager.allocator.dupe(u8, package_json_writer2.ctx.writtenWithoutTrailingZero()); + } + try manager.installWithManager(ctx, root_package_json.source.contents, log_level); if (subcommand == .update or subcommand == .add or subcommand == .link) { @@ -10255,17 +10283,19 @@ pub const PackageManager = struct { } if (manager.options.do.write_package_json) { + const source, const path = if (manager.options.patch_features == .commit) .{ root_package_json.source.contents, root_package_json_pathZ } else .{ new_package_json_source, manager.original_package_json_path }; + // 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( bun.invalid_fd, - manager.original_package_json_path, + path, bun.O.RDWR, 0, ).unwrap()).handle.asFile(); - try workspace_package_json_file.pwriteAll(new_package_json_source, 0); - std.posix.ftruncate(workspace_package_json_file.handle, new_package_json_source.len) catch {}; + try workspace_package_json_file.pwriteAll(source, 0); + std.posix.ftruncate(workspace_package_json_file.handle, source.len) catch {}; workspace_package_json_file.close(); if (subcommand == .remove) { @@ -10402,16 +10432,20 @@ pub const PackageManager = struct { name_and_version, pub fn fromArg(argument: []const u8) PatchArgKind { - if (bun.strings.hasPrefixComptime(argument, "node_modules/")) return .path; - if (bun.path.Platform.auto.isAbsolute(argument) and bun.strings.contains(argument, "node_modules/")) return .path; - if (comptime bun.Environment.isWindows) { - if (bun.strings.hasPrefix(argument, "node_modules\\")) return .path; - if (bun.path.Platform.auto.isAbsolute(argument) and bun.strings.contains(argument, "node_modules\\")) return .path; - } + if (bun.strings.containsComptime(argument, "node_modules/")) return .path; + if (bun.Environment.isWindows and bun.strings.hasPrefix(argument, "node_modules\\")) return .path; return .name_and_version; } }; + fn pathArgumentRelativeToRootWorkspacePackage(manager: *PackageManager, lockfile: *const Lockfile, argument: []const u8) ?[]const u8 { + const workspace_package_id = manager.root_package_id.get(lockfile, manager.workspace_name_hash); + if (workspace_package_id == 0) return null; + const workspace_res = lockfile.packages.items(.resolution)[workspace_package_id]; + const rel_path: []const u8 = workspace_res.value.workspace.slice(lockfile.buffers.string_bytes.items); + return bun.default_allocator.dupe(u8, bun.path.join(&[_][]const u8{ rel_path, argument }, .posix)) catch bun.outOfMemory(); + } + /// 1. Arg is either: /// - name and possibly version (e.g. "is-even" or "is-even@1.0.0") /// - path to package in node_modules @@ -10420,7 +10454,7 @@ pub const PackageManager = struct { /// 4. Print to user fn preparePatch(manager: *PackageManager) !void { const strbuf = manager.lockfile.buffers.string_bytes.items; - const argument = manager.options.positionals[1]; + var argument = manager.options.positionals[1]; const arg_kind: PatchArgKind = PatchArgKind.fromArg(argument); @@ -10430,9 +10464,24 @@ pub const PackageManager = struct { var win_normalizer: if (bun.Environment.isWindows) bun.PathBuffer else struct {} = undefined; + const not_in_workspace_root = manager.root_package_id.get(manager.lockfile, manager.workspace_name_hash) != 0; + var free_argument = false; + argument = if (arg_kind == .path and + not_in_workspace_root and + (!bun.path.Platform.posix.isAbsolute(argument) or (bun.Environment.isWindows and !bun.path.Platform.windows.isAbsolute(argument)))) + brk: { + if (pathArgumentRelativeToRootWorkspacePackage(manager, manager.lockfile, argument)) |rel_path| { + free_argument = true; + break :brk rel_path; + } + break :brk argument; + } else argument; + defer if (free_argument) manager.allocator.free(argument); + const cache_dir: std.fs.Dir, const cache_dir_subpath: []const u8, const module_folder: []const u8, const pkg_name: []const u8 = switch (arg_kind) { .path => brk: { var lockfile = manager.lockfile; + const package_json_source: logger.Source = src: { const package_json_path = bun.path.joinZ(&[_][]const u8{ argument, "package.json" }, .auto); @@ -10592,8 +10641,14 @@ pub const PackageManager = struct { Global.crash(); }; - Output.pretty("\nTo patch {s}, edit the following folder:\n\n {s}\n", .{ pkg_name, module_folder }); - Output.pretty("\nOnce you're done with your changes, run:\n\n bun patch --commit '{s}'\n", .{module_folder}); + if (not_in_workspace_root) { + var bufn: bun.PathBuffer = undefined; + Output.pretty("\nTo patch {s}, edit the following folder:\n\n {s}\n", .{ pkg_name, bun.path.joinStringBuf(bufn[0..], &[_][]const u8{ bun.fs.FileSystem.instance.topLevelDirWithoutTrailingSlash(), module_folder }, .posix) }); + Output.pretty("\nOnce you're done with your changes, run:\n\n bun patch --commit '{s}'\n", .{bun.path.joinStringBuf(bufn[0..], &[_][]const u8{ bun.fs.FileSystem.instance.topLevelDirWithoutTrailingSlash(), module_folder }, .posix)}); + } else { + Output.pretty("\nTo patch {s}, edit the following folder:\n\n {s}\n", .{ pkg_name, module_folder }); + Output.pretty("\nOnce you're done with your changes, run:\n\n bun patch --commit '{s}'\n", .{module_folder}); + } return; } @@ -10768,6 +10823,7 @@ pub const PackageManager = struct { const PatchCommitResult = struct { patch_key: []const u8, patchfile_path: []const u8, + not_in_workspace_root: bool = false, }; /// - Arg is the dir containing the package with changes OR name and version @@ -10818,10 +10874,23 @@ pub const PackageManager = struct { .ok => {}, } - const argument = manager.options.positionals[1]; - + var argument = manager.options.positionals[1]; const arg_kind: PatchArgKind = PatchArgKind.fromArg(argument); + const not_in_workspace_root = manager.root_package_id.get(lockfile, manager.workspace_name_hash) != 0; + var free_argument = false; + argument = if (arg_kind == .path and + not_in_workspace_root and + (!bun.path.Platform.posix.isAbsolute(argument) or (bun.Environment.isWindows and !bun.path.Platform.windows.isAbsolute(argument)))) + brk: { + if (pathArgumentRelativeToRootWorkspacePackage(manager, lockfile, argument)) |rel_path| { + free_argument = true; + break :brk rel_path; + } + break :brk argument; + } else argument; + defer if (free_argument) manager.allocator.free(argument); + // Attempt to open the existing node_modules folder var root_node_modules = switch (bun.sys.openatOSPath(bun.FD.cwd(), bun.OSPathLiteral("node_modules"), bun.O.DIRECTORY | bun.O.RDONLY, 0o755)) { .result => |fd| std.fs.Dir{ .fd = fd.cast() }, @@ -10980,7 +11049,7 @@ pub const PackageManager = struct { }, .posix); }; - const random_tempdir = bun.span(bun.fs.FileSystem.instance.tmpname(name, buf2[0..], bun.fastRandom()) catch |e| { + const random_tempdir = bun.span(bun.fs.FileSystem.instance.tmpname("node_modules_tmp", buf2[0..], bun.fastRandom()) catch |e| { Output.prettyError( "error: failed to make tempdir {s}\n", .{@errorName(e)}, @@ -11024,7 +11093,7 @@ pub const PackageManager = struct { break :has_nested_node_modules true; }; - const patch_tag_tmpname = bun.span(bun.fs.FileSystem.instance.tmpname(name, buf3[0..], bun.fastRandom()) catch |e| { + const patch_tag_tmpname = bun.span(bun.fs.FileSystem.instance.tmpname("patch_tmp", buf3[0..], bun.fastRandom()) catch |e| { Output.prettyError( "error: failed to make tempdir {s}\n", .{@errorName(e)}, @@ -11309,6 +11378,7 @@ pub const PackageManager = struct { return .{ .patch_key = patch_key, .patchfile_path = patchfile_path, + .not_in_workspace_root = not_in_workspace_root, }; } diff --git a/src/string_immutable.zig b/src/string_immutable.zig index ec37a6934ebbf5..c46e95875b6a6d 100644 --- a/src/string_immutable.zig +++ b/src/string_immutable.zig @@ -131,14 +131,19 @@ pub fn indexOfAny16(self: []const u16, comptime str: anytype) ?OptionalUsize { return null; } pub inline fn containsComptime(self: string, comptime str: string) bool { - var remain = self; + if (comptime str.len == 0) @compileError("Don't call this with an empty string plz."); + + const start = std.mem.indexOfScalar(u8, self, str[0]) orelse return false; + var remain = self[start..]; const Int = std.meta.Int(.unsigned, str.len * 8); while (remain.len >= comptime str.len) { if (@as(Int, @bitCast(remain.ptr[0..str.len].*)) == @as(Int, @bitCast(str.ptr[0..str.len].*))) { return true; } - remain = remain[str.len..]; + + const next_start = std.mem.indexOfScalar(u8, remain[1..], str[0]) orelse return false; + remain = remain[1 + next_start ..]; } return false; diff --git a/test/cli/install/bun-patch.test.ts b/test/cli/install/bun-patch.test.ts index e4655b7660c149..37bd10c27f9520 100644 --- a/test/cli/install/bun-patch.test.ts +++ b/test/cli/install/bun-patch.test.ts @@ -12,6 +12,156 @@ beforeAll(() => { }); describe("bun patch ", async () => { + describe("workspace interactions", async () => { + describe("inside workspace package", async () => { + const args = [ + ["node_modules/@types/ws", "packages/eslint-config/node_modules/@types/ws"], + ["@types/ws@8.5.4", "node_modules/@repo/eslint-config/node_modules/@types/ws"], + ]; + for (const [arg, path] of args) { + test(arg, async () => { + const tempdir = tempDirWithFiles("lol", { + "package.json": JSON.stringify({ + "name": "my-workspace", + private: "true", + version: "0.0.1", + "devDependencies": { + "@repo/ui": "*", + "@repo/eslint-config": "*", + "@repo/typescript-config": "*", + "@types/ws": "7.4.7", + }, + workspaces: ["packages/*"], + }), + packages: { + "eslint-config": { + "package.json": JSON.stringify({ + name: "@repo/eslint-config", + "version": "0.0.0", + dependencies: { + "@types/ws": "8.5.4", + }, + private: "true", + }), + }, + "typescript-config": { + "package.json": JSON.stringify({ + "name": "@repo/typescript-config", + "version": "0.0.0", + private: "true", + }), + }, + "ui": { + "package.json": JSON.stringify({ + name: "@repo/ui", + version: "0.0.0", + private: "true", + devDependencies: { + "@repo/eslint-config": "*", + "@repo/typescript-config": "*", + }, + }), + }, + }, + }); + + console.log("TEMPDIR", tempdir); + + await $`${bunExe()} i`.env(bunEnv).cwd(tempdir); + + let result = await $`cd packages/eslint-config; ${bunExe()} patch ${arg}`.env(bunEnv).cwd(tempdir); + expect(result.stderr.toString()).not.toContain("error"); + expect(result.stdout.toString()).toContain( + `To patch @types/ws, edit the following folder:\n\n ${tempdir}/${path}\n`, + ); + + await $`echo LOL > ${path}/index.js`.env(bunEnv).cwd(tempdir); + + expectNoError(await $`cd packages/eslint-config; ${bunExe()} patch --commit ${arg}`.env(bunEnv).cwd(tempdir)); + + expect(await $`cat ${path}/index.js`.env(bunEnv).cwd(tempdir).text()).toEqual("LOL\n"); + + expect( + (await $`cat package.json`.cwd(tempdir).env(bunEnv).json()).patchedDependencies["@types/ws@8.5.4"], + ).toEqual("patches/@types%2Fws@8.5.4.patch"); + }); + } + }); + + describe("inside ROOT workspace package", async () => { + const args = [ + ["packages/eslint-config/node_modules/@types/ws", "packages/eslint-config/node_modules/@types/ws"], + ["@types/ws@8.5.4", "node_modules/@repo/eslint-config/node_modules/@types/ws"], + ]; + for (const [arg, path] of args) { + test(arg, async () => { + const tempdir = tempDirWithFiles("lol", { + "package.json": JSON.stringify({ + "name": "my-workspace", + private: "true", + version: "0.0.1", + "devDependencies": { + "@repo/ui": "*", + "@repo/eslint-config": "*", + "@repo/typescript-config": "*", + "@types/ws": "7.4.7", + }, + workspaces: ["packages/*"], + }), + packages: { + "eslint-config": { + "package.json": JSON.stringify({ + name: "@repo/eslint-config", + "version": "0.0.0", + dependencies: { + "@types/ws": "8.5.4", + }, + private: "true", + }), + }, + "typescript-config": { + "package.json": JSON.stringify({ + "name": "@repo/typescript-config", + "version": "0.0.0", + private: "true", + }), + }, + "ui": { + "package.json": JSON.stringify({ + name: "@repo/ui", + version: "0.0.0", + private: "true", + devDependencies: { + "@repo/eslint-config": "*", + "@repo/typescript-config": "*", + }, + }), + }, + }, + }); + + console.log("TEMPDIR", tempdir); + + await $`${bunExe()} i`.env(bunEnv).cwd(tempdir); + + let result = await $`${bunExe()} patch ${arg}`.env(bunEnv).cwd(tempdir); + expect(result.stderr.toString()).not.toContain("error"); + expect(result.stdout.toString()).toContain(`To patch @types/ws, edit the following folder:\n\n ${path}\n`); + + await $`echo LOL > ${path}/index.js`.env(bunEnv).cwd(tempdir); + + expectNoError(await $`${bunExe()} patch --commit ${arg}`.env(bunEnv).cwd(tempdir)); + + expect(await $`cat ${path}/index.js`.env(bunEnv).cwd(tempdir).text()).toEqual("LOL\n"); + + expect( + (await $`cat package.json`.cwd(tempdir).env(bunEnv).json()).patchedDependencies["@types/ws@8.5.4"], + ).toEqual("patches/@types%2Fws@8.5.4.patch"); + }); + } + }); + }); + // Tests to make sure that patching describe("popular pkg", async () => { const dummyCode = /* ts */ `