fix(install): fix optional peer resolving

src/install/isolated_install.zig

@@ -250,7 +250,7 @@ pub fn installIsolatedPackages(
                 }
             }
 
-            next_peer: for (peer_dep_ids.items) |peer_dep_id| {
+            for (peer_dep_ids.items) |peer_dep_id| {
                 const resolved_pkg_id, const auto_installed = resolved_pkg_id: {
 
                     // Go through the peers parents looking for a package with the same name.
@@ -316,13 +316,12 @@ pub fn installIsolatedPackages(
                             // version. Only mark all parents if resolution is
                             // different from this transitive peer.
 
-                            if (peer_dep.behavior.isOptionalPeer()) {
-                                // exclude it
-                                continue :next_peer;
-                            }
-
                             const best_version = resolutions[peer_dep_id];
 
+                            if (best_version == invalid_package_id) {
+                                break :resolved_pkg_id .{ invalid_package_id, true };
+                            }
+
                             if (best_version == ids.pkg_id) {
                                 break :resolved_pkg_id .{ ids.pkg_id, true };
                             }
@@ -344,16 +343,15 @@ pub fn installIsolatedPackages(
                         curr_id = node_parent_ids[curr_id.get()];
                     }
 
-                    if (peer_dep.behavior.isOptionalPeer()) {
-                        // exclude it
-                        continue;
-                    }
-
                     // choose the current best version
                     break :resolved_pkg_id .{ resolutions[peer_dep_id], true };
                 };
 
-                bun.debugAssert(resolved_pkg_id != invalid_package_id);
+                if (resolved_pkg_id == invalid_package_id) {
+                    // these are optional peers that failed to find any dependency with a matching
+                    // name. they are completely excluded
+                    continue;
+                }
 
                 for (visited_parent_node_ids.items) |visited_parent_id| {
                     const ctx: Store.Node.TransitivePeer.OrderedArraySetCtx = .{

src/install/lockfile.zig

@@ -914,7 +914,6 @@ pub fn hoist(
     var slice = lockfile.packages.slice();
 
     var builder = Tree.Builder(method){
-        .name_hashes = slice.items(.name_hash),
         .queue = .init(allocator),
         .resolution_lists = slice.items(.resolutions),
         .resolutions = lockfile.buffers.resolutions.items,
@@ -926,6 +925,7 @@ pub fn hoist(
         .install_root_dependencies = install_root_dependencies,
         .workspace_filters = workspace_filters,
         .packages_to_install = packages_to_install,
+        .pending_optional_peers = .init(bun.default_allocator),
     };
 
     try (Tree{}).processSubtree(

src/install/lockfile/Tree.zig

@@ -52,14 +52,20 @@ pub const invalid_id: Id = std.math.maxInt(Id);
 pub const HoistDependencyResult = union(enum) {
     dependency_loop,
     hoisted,
-    placement: struct {
+    resolve: PackageID,
+    resolve_replace: ResolveReplace,
+    resolve_later,
+    placement: Placement,
+
+    const ResolveReplace = struct {
+        id: Id,
+        dep_id: DependencyID,
+    };
+
+    const Placement = struct {
         id: Id,
         bundled: bool = false,
-    },
-    // replace: struct {
-    //     dest_id: Id,
-    //     dep_id: DependencyID,
-    // },
+    };
 };
 
 pub const SubtreeError = OOM || error{DependencyLoop};
@@ -234,14 +240,16 @@ pub const BuilderMethod = enum {
 pub fn Builder(comptime method: BuilderMethod) type {
     return struct {
         allocator: Allocator,
-        name_hashes: []const PackageNameHash,
         list: bun.MultiArrayList(Entry) = .{},
-        resolutions: []const PackageID,
+        resolutions: []PackageID,
         dependencies: []const Dependency,
         resolution_lists: []const Lockfile.DependencyIDSlice,
         queue: TreeFiller,
         log: *logger.Log,
         lockfile: *const Lockfile,
+        // unresolved optional peers that might resolve later. if they do we will want to assign
+        // builder.resolutions[peer.dep_id] to the resolved pkg_id.
+        pending_optional_peers: std.AutoHashMap(PackageNameHash, bun.collections.ArrayListDefault(DependencyID)),
         manager: if (method == .filter) *const PackageManager else void,
         sort_buf: std.ArrayListUnmanaged(DependencyID) = .{},
         workspace_filters: if (method == .filter) []const WorkspaceFilter else void = if (method == .filter) &.{},
@@ -302,6 +310,7 @@ pub fn Builder(comptime method: BuilderMethod) type {
             }
             this.queue.deinit();
             this.sort_buf.deinit(this.allocator);
+            this.pending_optional_peers.deinit();
 
             // take over the `builder.list` pointer for only trees
             if (@intFromPtr(trees.ptr) != @intFromPtr(list_ptr)) {
@@ -328,6 +337,10 @@ pub fn isFilteredDependencyOrWorkspace(
 ) bool {
     const pkg_id = lockfile.buffers.resolutions.items[dep_id];
     if (pkg_id >= lockfile.packages.len) {
+        const dep = lockfile.buffers.dependencies.items[dep_id];
+        if (dep.behavior.isOptionalPeer()) {
+            return false;
+        }
         return true;
     }
 
@@ -454,8 +467,6 @@ pub fn processSubtree(
     const trees = list_slice.items(.tree);
     const dependency_lists = list_slice.items(.dependencies);
     const next: *Tree = &trees[builder.list.len - 1];
-    const name_hashes: []const PackageNameHash = builder.name_hashes;
-    const max_package_id = @as(PackageID, @truncate(name_hashes.len));
 
     const pkgs = builder.lockfile.packages.slice();
     const pkg_resolutions = pkgs.items(.resolution);
@@ -478,8 +489,6 @@ pub fn processSubtree(
 
     for (builder.sort_buf.items) |dep_id| {
         const pkg_id = builder.resolutions[dep_id];
-        // Skip unresolved packages, e.g. "peerDependencies"
-        if (pkg_id >= max_package_id) continue;
 
         // filter out disabled dependencies
         if (comptime method == .filter) {
@@ -494,6 +503,12 @@ pub fn processSubtree(
                 continue;
             }
 
+            // unresolved packages are skipped when filtering. they already had
+            // their chance to resolve.
+            if (pkg_id == invalid_package_id) {
+                continue;
+            }
+
             if (builder.packages_to_install) |packages_to_install| {
                 if (parent_pkg_id == 0) {
                     var found = false;
@@ -511,14 +526,33 @@ pub fn processSubtree(
             }
         }
 
+        const dependency = builder.dependencies[dep_id];
+
         const hoisted: HoistDependencyResult = hoisted: {
-            const dependency = builder.dependencies[dep_id];
 
             // don't hoist if it's a folder dependency or a bundled dependency.
             if (dependency.behavior.isBundled()) {
                 break :hoisted .{ .placement = .{ .id = next.id, .bundled = true } };
             }
 
+            if (pkg_id == invalid_package_id) {
+                if (dependency.behavior.isOptionalPeer()) {
+                    break :hoisted try next.hoistDependency(
+                        true,
+                        hoist_root_id,
+                        pkg_id,
+                        &dependency,
+                        dependency_lists,
+                        trees,
+                        method,
+                        builder,
+                    );
+                }
+
+                // skip unresolvable dependencies
+                continue;
+            }
+
             if (pkg_resolutions[pkg_id].tag == .folder) {
                 break :hoisted .{ .placement = .{ .id = next.id } };
             }
@@ -537,10 +571,61 @@ pub fn processSubtree(
 
         switch (hoisted) {
             .dependency_loop, .hoisted => continue,
+
+            .resolve => |res_id| {
+                bun.assertWithLocation(pkg_id == invalid_package_id, @src());
+                bun.assertWithLocation(res_id != invalid_package_id, @src());
+                builder.resolutions[dep_id] = res_id;
+                if (comptime Environment.allow_assert) {
+                    bun.assertWithLocation(!builder.pending_optional_peers.contains(dependency.name_hash), @src());
+                }
+                if (builder.pending_optional_peers.fetchRemove(dependency.name_hash)) |entry| {
+                    var peers = entry.value;
+                    defer peers.deinit();
+                    for (peers.items()) |unresolved_dep_id| {
+                        bun.assertWithLocation(builder.resolutions[unresolved_dep_id] == invalid_package_id, @src());
+                        builder.resolutions[unresolved_dep_id] = res_id;
+                    }
+                }
+            },
+            .resolve_replace => |replace| {
+                bun.assertWithLocation(pkg_id != invalid_package_id, @src());
+                builder.resolutions[replace.dep_id] = pkg_id;
+                if (builder.pending_optional_peers.fetchRemove(dependency.name_hash)) |entry| {
+                    var peers = entry.value;
+                    defer peers.deinit();
+                    for (peers.items()) |unresolved_dep_id| {
+                        bun.assertWithLocation(builder.resolutions[unresolved_dep_id] == invalid_package_id, @src());
+                        builder.resolutions[unresolved_dep_id] = pkg_id;
+                    }
+                }
+                for (dependency_lists[replace.id].items) |*placed_dep_id| {
+                    if (placed_dep_id.* == replace.dep_id) {
+                        placed_dep_id.* = dep_id;
+                    }
+                }
+                if (pkg_id != invalid_package_id and builder.resolution_lists[pkg_id].len > 0) {
+                    try builder.queue.writeItem(.{
+                        .tree_id = replace.id,
+                        .dependency_id = dep_id,
+                        .hoist_root_id = hoist_root_id,
+                    });
+                }
+            },
+            .resolve_later => {
+                // `dep_id` is an unresolved optional peer. while hoisting it deduplicated
+                // with another unresolved optional peer. save it so we remember resolve it
+                // later if it's possible to resolve it.
+                const entry = try builder.pending_optional_peers.getOrPut(dependency.name_hash);
+                if (!entry.found_existing) {
+                    entry.value_ptr.* = .init();
+                }
+                try entry.value_ptr.append(dep_id);
+            },
             .placement => |dest| {
                 bun.handleOom(dependency_lists[dest.id].append(builder.allocator, dep_id));
                 trees[dest.id].dependencies.len += 1;
-                if (builder.resolution_lists[pkg_id].len > 0) {
+                if (pkg_id != invalid_package_id and builder.resolution_lists[pkg_id].len > 0) {
                     try builder.queue.writeItem(.{
                         .tree_id = dest.id,
                         .dependency_id = dep_id,
@@ -580,7 +665,29 @@ fn hoistDependency(
         const dep = builder.dependencies[dep_id];
         if (dep.name_hash != dependency.name_hash) continue;
 
-        if (builder.resolutions[dep_id] == package_id) {
+        const res_id = builder.resolutions[dep_id];
+
+        if (res_id == invalid_package_id and package_id == invalid_package_id) {
+            bun.assertWithLocation(dep.behavior.isOptionalPeer(), @src());
+            bun.assertWithLocation(dependency.behavior.isOptionalPeer(), @src());
+            // both optional peers will need to be resolved if they can resolve later.
+            // remember input package_id and dependency for later
+            return .resolve_later;
+        }
+
+        if (res_id == invalid_package_id) {
+            bun.assertWithLocation(dep.behavior.isOptionalPeer(), @src());
+            return .{ .resolve_replace = .{ .id = this.id, .dep_id = dep_id } };
+        }
+
+        if (package_id == invalid_package_id) {
+            bun.assertWithLocation(dependency.behavior.isOptionalPeer(), @src());
+            bun.assertWithLocation(res_id != invalid_package_id, @src());
+            // resolve optional peer to `builder.resolutions[dep_id]`
+            return .{ .resolve = res_id }; // 1
+        }
+
+        if (res_id == package_id) {
             // this dependency is the same package as the other, hoist
             return .hoisted; // 1
         }
@@ -599,7 +706,7 @@ fn hoistDependency(
 
         if (dependency.behavior.isPeer()) {
             if (dependency.version.tag == .npm) {
-                const resolution: Resolution = builder.lockfile.packages.items(.resolution)[builder.resolutions[dep_id]];
+                const resolution: Resolution = builder.lockfile.packages.items(.resolution)[res_id];
                 const version = dependency.version.value.npm.version;
                 if (resolution.tag == .npm and version.satisfies(resolution.value.npm.version, builder.buf(), builder.buf())) {
                     return .hoisted; // 1
@@ -618,8 +725,8 @@ fn hoistDependency(
             builder.maybeReportError("Package \"{}@{}\" has a dependency loop\n  Resolution: \"{}@{}\"\n  Dependency: \"{}@{}\"", .{
                 builder.packageName(package_id),
                 builder.packageVersion(package_id),
-                builder.packageName(builder.resolutions[dep_id]),
-                builder.packageVersion(builder.resolutions[dep_id]),
+                builder.packageName(res_id),
+                builder.packageVersion(res_id),
                 dependency.name.fmt(builder.buf()),
                 dependency.version.literal.fmt(builder.buf()),
             });