From 37bd79682e81722e08eaeac7e009b20fe9d64bf6 Mon Sep 17 00:00:00 2001 From: Dylan Conway Date: Thu, 10 Jul 2025 00:57:04 -0700 Subject: [PATCH] sort and workspace only --- src/install/dependency.zig | 12 +- src/install/lockfile/Package.zig | 4 +- src/install/lockfile/bun.lock.zig | 26 +- .../test-dev-peer-dependency-priority.test.ts | 323 ++++++++++++++++++ 4 files changed, 353 insertions(+), 12 deletions(-) create mode 100644 test/cli/install/test-dev-peer-dependency-priority.test.ts diff --git a/src/install/dependency.zig b/src/install/dependency.zig index 4ea40ac399..d279bbed26 100644 --- a/src/install/dependency.zig +++ b/src/install/dependency.zig @@ -1413,15 +1413,15 @@ pub const Behavior = packed struct(u8) { .gt; } - if (lhs.isProd() != rhs.isProd()) { - return if (lhs.isProd()) + if (lhs.isPeer() != rhs.isPeer()) { + return if (lhs.isPeer()) .gt else .lt; } - if (lhs.isDev() != rhs.isDev()) { - return if (lhs.isDev()) + if (lhs.isProd() != rhs.isProd()) { + return if (lhs.isProd()) .gt else .lt; @@ -1434,8 +1434,8 @@ pub const Behavior = packed struct(u8) { .lt; } - if (lhs.isPeer() != rhs.isPeer()) { - return if (lhs.isPeer()) + if (lhs.isDev() != rhs.isDev()) { + return if (lhs.isDev()) .gt else .lt; diff --git a/src/install/lockfile/Package.zig b/src/install/lockfile/Package.zig index 8edae3a083..6cb0e5d5f7 100644 --- a/src/install/lockfile/Package.zig +++ b/src/install/lockfile/Package.zig @@ -1112,7 +1112,7 @@ pub const Package = extern struct { } else { // It doesn't satisfy, but a workspace shares the same name. Override the workspace with the other dependency for (package_dependencies[0..dependencies_count]) |*dep| { - if (dep.name_hash == name_hash and dep.version.tag == .workspace) { + if (dep.name_hash == name_hash and dep.behavior.isWorkspaceOnly()) { dep.* = .{ .behavior = if (in_workspace) group.behavior.add(.workspace) else group.behavior, .name = external_alias.value, @@ -2141,6 +2141,8 @@ pub const Package = extern struct { }; }; +// @sortImports + const TrustedDependenciesSet = Lockfile.TrustedDependenciesSet; const Aligner = install.Aligner; const Allocator = std.mem.Allocator; diff --git a/src/install/lockfile/bun.lock.zig b/src/install/lockfile/bun.lock.zig index db50bdf193..14de2868d1 100644 --- a/src/install/lockfile/bun.lock.zig +++ b/src/install/lockfile/bun.lock.zig @@ -1904,6 +1904,9 @@ pub fn parseIntoBinaryLockfile( lockfile.buffers.resolutions.expandToCapacity(); @memset(lockfile.buffers.resolutions.items, invalid_package_id); + var seen_deps: bun.StringHashMap(void) = .init(allocator); + defer seen_deps.deinit(); + const pkgs = lockfile.packages.slice(); const pkg_deps = pkgs.items(.dependencies); const pkg_names = pkgs.items(.name); @@ -1919,6 +1922,8 @@ pub fn parseIntoBinaryLockfile( const dep_id: DependencyID = @intCast(_dep_id); const dep = &lockfile.buffers.dependencies.items[dep_id]; + const is_duplicate_dep = (try seen_deps.getOrPut(dep.name.slice(lockfile.buffers.string_bytes.items))).found_existing; + const res_id = pkg_map.get(dep.name.slice(lockfile.buffers.string_bytes.items)) orelse { if (dep.behavior.optional) { continue; @@ -1927,7 +1932,7 @@ pub fn parseIntoBinaryLockfile( return error.InvalidPackageInfo; }; - mapDepToPkg(dep, dep_id, res_id, lockfile, pkg_resolutions); + mapDepToPkg(dep, dep_id, res_id, lockfile, pkg_resolutions, is_duplicate_dep); } } @@ -1939,12 +1944,16 @@ pub fn parseIntoBinaryLockfile( const pkg_id: PackageID = @intCast(_pkg_id); const workspace_name = pkg_names[pkg_id].slice(lockfile.buffers.string_bytes.items); + seen_deps.clearRetainingCapacity(); + const deps = pkg_deps[pkg_id]; for (deps.begin()..deps.end()) |_dep_id| { const dep_id: DependencyID = @intCast(_dep_id); const dep = &lockfile.buffers.dependencies.items[dep_id]; const dep_name = dep.name.slice(lockfile.buffers.string_bytes.items); + const is_duplicate_dep = (try seen_deps.getOrPut(dep_name)).found_existing; + const workspace_node_modules = std.fmt.bufPrint(&path_buf, "{s}/{s}", .{ workspace_name, dep_name }) catch { try log.addErrorFmt(source, root_pkg_exr.loc, allocator, "Workspace and dependency name too long: '{s}/{s}'", .{ workspace_name, dep_name }); return error.InvalidPackageInfo; @@ -1958,7 +1967,7 @@ pub fn parseIntoBinaryLockfile( return error.InvalidPackageInfo; }; - mapDepToPkg(dep, dep_id, res_id, lockfile, pkg_resolutions); + mapDepToPkg(dep, dep_id, res_id, lockfile, pkg_resolutions, is_duplicate_dep); } } } @@ -1973,12 +1982,16 @@ pub fn parseIntoBinaryLockfile( return error.InvalidPackagesObject; }; + seen_deps.clearRetainingCapacity(); + // find resolutions. iterate up to root through the pkg path. const deps = pkg_deps[pkg_id]; deps: for (deps.begin()..deps.end()) |_dep_id| { const dep_id: DependencyID = @intCast(_dep_id); const dep = &lockfile.buffers.dependencies.items[dep_id]; + const is_duplicate_dep = (try seen_deps.getOrPut(dep.name.slice(lockfile.buffers.string_bytes.items))).found_existing; + const res_id = pkg_map.findResolution(pkg_path, dep, lockfile.buffers.string_bytes.items, &path_buf) catch |err| switch (err) { error.InvalidPackageKey => { try log.addError(source, key.loc, "Invalid package path"); @@ -1993,7 +2006,7 @@ pub fn parseIntoBinaryLockfile( }, }; - mapDepToPkg(dep, dep_id, res_id, lockfile, pkg_resolutions); + mapDepToPkg(dep, dep_id, res_id, lockfile, pkg_resolutions, is_duplicate_dep); } } @@ -2008,12 +2021,15 @@ pub fn parseIntoBinaryLockfile( } } -fn mapDepToPkg(dep: *Dependency, dep_id: DependencyID, pkg_id: PackageID, lockfile: *BinaryLockfile, pkg_resolutions: []const Resolution) void { +fn mapDepToPkg(dep: *Dependency, dep_id: DependencyID, pkg_id: PackageID, lockfile: *BinaryLockfile, pkg_resolutions: []const Resolution, is_duplicate_dep: bool) void { lockfile.buffers.resolutions.items[dep_id] = pkg_id; if (lockfile.text_lockfile_version != .v0) { const res = &pkg_resolutions[pkg_id]; - if (res.tag == .workspace) { + + // when a package has duplicate dependencies and one is a workspace + // we don't want to override the other because it might not be a workspace. + if (res.tag == .workspace and !is_duplicate_dep) { dep.version.tag = .workspace; dep.version.value = .{ .workspace = res.value.workspace }; diff --git a/test/cli/install/test-dev-peer-dependency-priority.test.ts b/test/cli/install/test-dev-peer-dependency-priority.test.ts new file mode 100644 index 0000000000..6c8708b9fe --- /dev/null +++ b/test/cli/install/test-dev-peer-dependency-priority.test.ts @@ -0,0 +1,323 @@ +import { expect, test } from "bun:test"; +import { bunEnv, bunExe, tempDirWithFiles } from "harness"; +import { join } from "path"; + +test("workspace devDependencies should take priority over peerDependencies for resolution", async () => { + const dir = tempDirWithFiles("dev-peer-priority", { + "package.json": JSON.stringify({ + name: "test-monorepo", + version: "1.0.0", + workspaces: { + packages: ["packages/*"], + nodeLinker: "isolated", + }, + }), + "packages/lib/package.json": JSON.stringify({ + name: "lib", + version: "1.0.0", + dependencies: {}, + devDependencies: { + "jquery": "workspace:*", // Use workspace protocol for dev + }, + peerDependencies: { + "jquery": "3.7.0", // Range wants 3.7.0 + }, + }), + "packages/lib/test.js": `const dep = require("jquery"); console.log(dep.version);`, + // Only provide workspace package with version 2.0.0 + "packages/my-dep/package.json": JSON.stringify({ + name: "jquery", + version: "2.0.0", + main: "index.js", + }), + "packages/my-dep/index.js": `module.exports = { version: "2.0.0" };`, + }); + + // Run initial install + let { stdout, stderr, exitCode } = await new Promise<{ stdout: string; stderr: string; exitCode: number }>( + resolve => { + const proc = Bun.spawn({ + cmd: [bunExe(), "install", "--no-progress", "--no-summary"], + cwd: dir, + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + proc.exited.then(exitCode => { + Promise.all([new Response(proc.stdout).text(), new Response(proc.stderr).text()]).then(([stdout, stderr]) => { + resolve({ stdout, stderr, exitCode }); + }); + }); + }, + ); + + if (exitCode !== 0) { + console.error("Install failed with exit code:", exitCode); + console.error("stdout:", stdout); + console.error("stderr:", stderr); + } + expect(exitCode).toBe(0); + + // Now run bun install with a dead registry to ensure no network requests + ({ stdout, stderr, exitCode } = await new Promise<{ stdout: string; stderr: string; exitCode: number }>(resolve => { + const proc = Bun.spawn({ + cmd: [bunExe(), "install", "--no-progress", "--no-summary"], + cwd: dir, + env: { + ...bunEnv, + NPM_CONFIG_REGISTRY: "http://localhost:9999/", // Dead URL - will fail if used + }, + stdout: "pipe", + stderr: "pipe", + }); + + proc.exited.then(exitCode => { + Promise.all([new Response(proc.stdout).text(), new Response(proc.stderr).text()]).then(([stdout, stderr]) => { + resolve({ stdout, stderr, exitCode }); + }); + }); + })); + + if (exitCode !== 0) { + console.error("Install failed with exit code:", exitCode); + console.error("stdout:", stdout); + console.error("stderr:", stderr); + } + expect(exitCode).toBe(0); + + // Check that no network requests were made for packages that should be resolved locally + expect(stderr).not.toContain("GET"); + expect(stderr).not.toContain("http"); + + // Check that the lockfile was created correctly + const lockfilePath = join(dir, "bun.lock"); + expect(await Bun.file(lockfilePath).exists()).toBe(true); + + // Verify that version 2.0.0 (devDependency) was linked + // If peerDependency range ^1.0.0 was used, it would try to fetch from npm and fail + const testResult = await new Promise(resolve => { + const proc = Bun.spawn({ + cmd: [bunExe(), "packages/lib/test.js"], + cwd: dir, + env: bunEnv, + stdout: "pipe", + }); + + new Response(proc.stdout).text().then(resolve); + }); + + expect(testResult.trim()).toBe("2.0.0"); +}); + +test("devDependencies and peerDependencies with different versions should coexist", async () => { + const dir = tempDirWithFiles("dev-peer-different-versions", { + "package.json": JSON.stringify({ + name: "test-monorepo", + version: "1.0.0", + workspaces: { + packages: ["packages/*"], + nodeLinker: "isolated", + }, + }), + "packages/lib/package.json": JSON.stringify({ + name: "lib", + version: "1.0.0", + dependencies: {}, + devDependencies: { + "utils": "1.0.0", + }, + peerDependencies: { + "utils": "^1.0.0", + }, + }), + "packages/lib/index.js": `console.log("lib");`, + "packages/utils/package.json": JSON.stringify({ + name: "utils", + version: "1.0.0", + main: "index.js", + }), + "packages/utils/index.js": `console.log("utils");`, + }); + + // Run bun install in the monorepo + const { stdout, stderr, exitCode } = await new Promise<{ stdout: string; stderr: string; exitCode: number }>( + resolve => { + const proc = Bun.spawn({ + cmd: [bunExe(), "install", "--no-progress", "--no-summary"], + cwd: dir, + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + proc.exited.then(exitCode => { + Promise.all([new Response(proc.stdout).text(), new Response(proc.stderr).text()]).then(([stdout, stderr]) => { + resolve({ stdout, stderr, exitCode }); + }); + }); + }, + ); + + if (exitCode !== 0) { + console.error("Install failed with exit code:", exitCode); + console.error("stdout:", stdout); + console.error("stderr:", stderr); + } + expect(exitCode).toBe(0); + + // Check that the lockfile was created correctly + const lockfilePath = join(dir, "bun.lock"); + expect(await Bun.file(lockfilePath).exists()).toBe(true); +}); + +test("dependency behavior comparison prioritizes devDependencies", async () => { + const dir = tempDirWithFiles("behavior-comparison", { + "package.json": JSON.stringify({ + name: "test-app", + version: "1.0.0", + dependencies: {}, + devDependencies: { + "typescript": "^5.0.0", + }, + peerDependencies: { + "typescript": "^4.0.0 || ^5.0.0", + }, + }), + "index.js": `console.log("app");`, + }); + + // Run bun install + const { stdout, stderr, exitCode } = await new Promise<{ stdout: string; stderr: string; exitCode: number }>( + resolve => { + const proc = Bun.spawn({ + cmd: [bunExe(), "install", "--no-progress", "--no-summary"], + cwd: dir, + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + proc.exited.then(exitCode => { + Promise.all([new Response(proc.stdout).text(), new Response(proc.stderr).text()]).then(([stdout, stderr]) => { + resolve({ stdout, stderr, exitCode }); + }); + }); + }, + ); + + if (exitCode !== 0) { + console.error("Install failed with exit code:", exitCode); + console.error("stdout:", stdout); + console.error("stderr:", stderr); + } + expect(exitCode).toBe(0); + + // Check that the lockfile was created correctly + const lockfilePath = join(dir, "bun.lock"); + expect(await Bun.file(lockfilePath).exists()).toBe(true); +}); + +test("Next.js monorepo scenario should not make unnecessary network requests", async () => { + const dir = tempDirWithFiles("nextjs-monorepo", { + "package.json": JSON.stringify({ + name: "nextjs-monorepo", + version: "1.0.0", + workspaces: { + packages: ["packages/*"], + nodeLinker: "isolated", + }, + }), + "packages/web/package.json": JSON.stringify({ + name: "web", + version: "1.0.0", + dependencies: {}, + devDependencies: { + "next": "15.0.0-canary.119", // Specific canary version for dev + }, + peerDependencies: { + "next": "^14.0.0 || ^15.0.0", // Range that would accept 14.x or 15.x stable + }, + }), + "packages/web/test.js": `const next = require("next/package.json"); console.log(next.version);`, + // Only provide the canary version that matches devDependencies + "packages/next/package.json": JSON.stringify({ + name: "next", + version: "15.0.0-canary.119", + main: "index.js", + }), + "packages/next/index.js": `console.log("next workspace");`, + }); + + // Run initial install + let { stdout, stderr, exitCode } = await new Promise<{ stdout: string; stderr: string; exitCode: number }>( + resolve => { + const proc = Bun.spawn({ + cmd: [bunExe(), "install", "--no-progress", "--no-summary"], + cwd: dir, + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + proc.exited.then(exitCode => { + Promise.all([new Response(proc.stdout).text(), new Response(proc.stderr).text()]).then(([stdout, stderr]) => { + resolve({ stdout, stderr, exitCode }); + }); + }); + }, + ); + + if (exitCode !== 0) { + console.error("Install failed with exit code:", exitCode); + console.error("stdout:", stdout); + console.error("stderr:", stderr); + } + expect(exitCode).toBe(0); + + // Run bun install with dead registry + ({ stdout, stderr, exitCode } = await new Promise<{ stdout: string; stderr: string; exitCode: number }>(resolve => { + const proc = Bun.spawn({ + cmd: [bunExe(), "install", "--no-progress", "--no-summary"], + cwd: dir, + env: { + ...bunEnv, + NPM_CONFIG_REGISTRY: "http://localhost:9999/", // Dead URL + }, + stdout: "pipe", + stderr: "pipe", + }); + + proc.exited.then(exitCode => { + Promise.all([new Response(proc.stdout).text(), new Response(proc.stderr).text()]).then(([stdout, stderr]) => { + resolve({ stdout, stderr, exitCode }); + }); + }); + })); + + expect(exitCode).toBe(0); + + // The key test: should not make network requests for packages that exist in workspace + // When devDependencies are prioritized over peerDependencies, the workspace version should be used + expect(stderr).not.toContain("GET"); + expect(stderr).not.toContain("404"); + expect(stderr).not.toContain("http"); + + // Check that the lockfile was created correctly + const lockfilePath = join(dir, "bun.lock"); + expect(await Bun.file(lockfilePath).exists()).toBe(true); + + // Verify that version 15.0.0-canary.119 (devDependency) was used + // If peer range was used, it would try to fetch a stable version from npm and fail + const testResult = await new Promise(resolve => { + const proc = Bun.spawn({ + cmd: [bunExe(), "packages/web/test.js"], + cwd: dir, + env: bunEnv, + stdout: "pipe", + }); + + new Response(proc.stdout).text().then(resolve); + }); + + expect(testResult.trim()).toBe("15.0.0-canary.119"); +});