mirror of
https://github.com/oven-sh/bun
synced 2026-02-13 20:39:05 +00:00
Fix npm override parsing - parent is key, children are nested values
After testing actual npm behavior, discovered the correct format is:
{
"overrides": {
"parent-pkg": {
"child-pkg": "version"
}
}
}
NOT what I originally implemented (child as key with parent nested inside).
Updated parseFromOverrides to correctly parse this structure. Override is now
stored in lockfile correctly, but runtime application still needs debugging.
Test results:
- Parsing works ✓ (override appears in lockfile)
- Runtime application ✗ (override not applied during install)
Next: Debug why parent_name_hash lookup isn't working in PackageManagerEnqueue
This commit is contained in:
Claude Bot
@@ -331,17 +331,17 @@ pub fn parseFromOverrides(
|
||||
|
||||
for (expr.data.e_object.properties.slice()) |prop| {
|
||||
const key = prop.key.?;
|
||||
const child_name = key.asString(lockfile.allocator).?;
|
||||
if (child_name.len == 0) {
|
||||
try log.addWarningFmt(source, key.loc, lockfile.allocator, "Missing overridden package name", .{});
|
||||
const package_name = key.asString(lockfile.allocator).?;
|
||||
if (package_name.len == 0) {
|
||||
try log.addWarningFmt(source, key.loc, lockfile.allocator, "Missing package name in overrides", .{});
|
||||
continue;
|
||||
}
|
||||
|
||||
const child_name_hash = String.Builder.stringHash(child_name);
|
||||
const value_expr = prop.value.?;
|
||||
|
||||
// Handle simple string override: "pkg": "1.0.0"
|
||||
// Handle simple string override: "pkg": "1.0.0" (global override)
|
||||
if (value_expr.data == .e_string) {
|
||||
const package_name_hash = String.Builder.stringHash(package_name);
|
||||
const version_str = value_expr.data.e_string.slice(lockfile.allocator);
|
||||
if (strings.hasPrefixComptime(version_str, "patch:")) {
|
||||
try log.addWarningFmt(source, key.loc, lockfile.allocator, "Bun currently does not support patched package \"overrides\"", .{});
|
||||
@@ -356,107 +356,82 @@ pub fn parseFromOverrides(
|
||||
source,
|
||||
value_expr.loc,
|
||||
log,
|
||||
child_name,
|
||||
package_name,
|
||||
version_str,
|
||||
builder,
|
||||
)) |dep| {
|
||||
this.map.putAssumeCapacity(child_name_hash, .{ .global = dep });
|
||||
this.map.putAssumeCapacity(package_name_hash, .{ .global = dep });
|
||||
}
|
||||
continue;
|
||||
}
|
||||
|
||||
// Handle nested overrides: "child-pkg": { ".": "1.0.0", "parent-pkg": { "child-pkg": "2.0.0" } }
|
||||
// The "." key provides a global override, other keys are parent package names
|
||||
// Handle object: could be either "parent": { "child": "version" } or global package with "."
|
||||
if (value_expr.data == .e_object) {
|
||||
var nested = NestedOverrides{};
|
||||
errdefer nested.deinit(lockfile.allocator);
|
||||
|
||||
const parent_name = package_name;
|
||||
const parent_name_hash = String.Builder.stringHash(parent_name);
|
||||
const nested_props = value_expr.data.e_object.properties.slice();
|
||||
try nested.parent_map.ensureTotalCapacity(lockfile.allocator, nested_props.len);
|
||||
|
||||
for (nested_props) |nested_prop| {
|
||||
const nested_key = nested_prop.key.?;
|
||||
const nested_key_str = nested_key.asString(lockfile.allocator).?;
|
||||
// Iterate through children of this parent
|
||||
for (nested_props) |child_prop| {
|
||||
const child_key = child_prop.key.?;
|
||||
const child_name = child_key.asString(lockfile.allocator).?;
|
||||
|
||||
// Handle "." which is the global override for this child package
|
||||
if (strings.eqlComptime(nested_key_str, ".")) {
|
||||
if (nested_prop.value.?.data != .e_string) {
|
||||
try log.addWarningFmt(source, nested_prop.value.?.loc, lockfile.allocator, "Invalid override value for \".\"", .{});
|
||||
continue;
|
||||
}
|
||||
if (child_prop.value.?.data != .e_string) {
|
||||
try log.addWarningFmt(source, child_prop.value.?.loc, lockfile.allocator, "Invalid override value for \"{s}\"", .{child_name});
|
||||
continue;
|
||||
}
|
||||
|
||||
const nested_version_str = nested_prop.value.?.data.e_string.slice(lockfile.allocator);
|
||||
if (strings.hasPrefixComptime(nested_version_str, "patch:")) {
|
||||
try log.addWarningFmt(source, nested_key.loc, lockfile.allocator, "Bun currently does not support patched package \"overrides\"", .{});
|
||||
continue;
|
||||
}
|
||||
const child_version_str = child_prop.value.?.data.e_string.slice(lockfile.allocator);
|
||||
if (strings.hasPrefixComptime(child_version_str, "patch:")) {
|
||||
try log.addWarningFmt(source, child_key.loc, lockfile.allocator, "Bun currently does not support patched package \"overrides\"", .{});
|
||||
continue;
|
||||
}
|
||||
|
||||
if (try parseOverrideValue(
|
||||
"override",
|
||||
lockfile,
|
||||
pm,
|
||||
root_package,
|
||||
source,
|
||||
nested_prop.value.?.loc,
|
||||
log,
|
||||
child_name,
|
||||
nested_version_str,
|
||||
builder,
|
||||
)) |dep| {
|
||||
nested.global = dep;
|
||||
}
|
||||
} else {
|
||||
// nested_key_str is a parent package name
|
||||
// The value must be an object with child-pkg as a key
|
||||
const parent_name = nested_key_str;
|
||||
const parent_name_hash = String.Builder.stringHash(parent_name);
|
||||
const child_name_hash = String.Builder.stringHash(child_name);
|
||||
|
||||
if (nested_prop.value.?.data != .e_object) {
|
||||
try log.addWarningFmt(source, nested_prop.value.?.loc, lockfile.allocator, "Invalid nested override for parent \"{s}\", expected object", .{parent_name});
|
||||
continue;
|
||||
}
|
||||
|
||||
// Look for the child package in the parent's overrides
|
||||
if (nested_prop.value.?.asProperty(child_name)) |child_override| {
|
||||
if (child_override.expr.data != .e_string) {
|
||||
try log.addWarningFmt(source, child_override.expr.loc, lockfile.allocator, "Invalid override value for \"{s}\"", .{child_name});
|
||||
continue;
|
||||
}
|
||||
|
||||
const override_version_str = child_override.expr.data.e_string.slice(lockfile.allocator);
|
||||
if (strings.hasPrefixComptime(override_version_str, "patch:")) {
|
||||
try log.addWarningFmt(source, child_override.expr.loc, lockfile.allocator, "Bun currently does not support patched package \"overrides\"", .{});
|
||||
continue;
|
||||
}
|
||||
|
||||
if (try parseOverrideValue(
|
||||
"override",
|
||||
lockfile,
|
||||
pm,
|
||||
root_package,
|
||||
source,
|
||||
child_override.expr.loc,
|
||||
log,
|
||||
child_name,
|
||||
override_version_str,
|
||||
builder,
|
||||
)) |dep| {
|
||||
nested.parent_map.putAssumeCapacity(parent_name_hash, dep);
|
||||
if (try parseOverrideValue(
|
||||
"override",
|
||||
lockfile,
|
||||
pm,
|
||||
root_package,
|
||||
source,
|
||||
child_prop.value.?.loc,
|
||||
log,
|
||||
child_name,
|
||||
child_version_str,
|
||||
builder,
|
||||
)) |dep| {
|
||||
// Get or create the nested override entry for this child
|
||||
const gop = try this.map.getOrPut(lockfile.allocator, child_name_hash);
|
||||
if (!gop.found_existing) {
|
||||
// Create new nested override
|
||||
var nested = NestedOverrides{};
|
||||
try nested.parent_map.ensureTotalCapacity(lockfile.allocator, 1);
|
||||
nested.parent_map.putAssumeCapacity(parent_name_hash, dep);
|
||||
gop.value_ptr.* = .{ .nested = nested };
|
||||
} else {
|
||||
// Update existing entry
|
||||
switch (gop.value_ptr.*) {
|
||||
.global => |global_dep| {
|
||||
// Convert global to nested, keeping the global as fallback
|
||||
var nested = NestedOverrides{};
|
||||
nested.global = global_dep;
|
||||
try nested.parent_map.ensureTotalCapacity(lockfile.allocator, 1);
|
||||
nested.parent_map.putAssumeCapacity(parent_name_hash, dep);
|
||||
gop.value_ptr.* = .{ .nested = nested };
|
||||
},
|
||||
.nested => |*nested| {
|
||||
// Add this parent-specific override
|
||||
try nested.parent_map.put(lockfile.allocator, parent_name_hash, dep);
|
||||
},
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Only add to map if we have at least a global or some nested overrides
|
||||
if (nested.global != null or nested.parent_map.count() > 0) {
|
||||
this.map.putAssumeCapacity(child_name_hash, .{ .nested = nested });
|
||||
} else {
|
||||
nested.deinit(lockfile.allocator);
|
||||
}
|
||||
continue;
|
||||
}
|
||||
|
||||
try log.addWarningFmt(source, value_expr.loc, lockfile.allocator, "Invalid override value for \"{s}\"", .{child_name});
|
||||
try log.addWarningFmt(source, value_expr.loc, lockfile.allocator, "Invalid override value for \"{s}\"", .{package_name});
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -249,7 +249,7 @@ test("overrides do not apply to workspaces", async () => {
|
||||
});
|
||||
|
||||
// NPM-style nested overrides tests
|
||||
test("nested overrides - npm format with global and parent-specific", async () => {
|
||||
test("nested overrides - npm format parent-specific", async () => {
|
||||
const tmp = tmpdirSync();
|
||||
writeFileSync(
|
||||
join(tmp, "package.json"),
|
||||
@@ -258,11 +258,8 @@ test("nested overrides - npm format with global and parent-specific", async () =
|
||||
express: "4.18.2",
|
||||
},
|
||||
overrides: {
|
||||
bytes: {
|
||||
".": "2.0.0",
|
||||
express: {
|
||||
bytes: "1.0.0",
|
||||
},
|
||||
express: {
|
||||
bytes: "1.0.0",
|
||||
},
|
||||
},
|
||||
}),
|
||||
@@ -275,7 +272,7 @@ test("nested overrides - npm format with global and parent-specific", async () =
|
||||
ensureLockfileDoesntChangeOnBunI(tmp);
|
||||
});
|
||||
|
||||
test("nested overrides - npm format with only parent-specific override", async () => {
|
||||
test("nested overrides - npm format with global and parent-specific", async () => {
|
||||
const tmp = tmpdirSync();
|
||||
writeFileSync(
|
||||
join(tmp, "package.json"),
|
||||
@@ -284,40 +281,16 @@ test("nested overrides - npm format with only parent-specific override", async (
|
||||
express: "4.18.2",
|
||||
},
|
||||
overrides: {
|
||||
bytes: {
|
||||
express: {
|
||||
bytes: "1.0.0",
|
||||
},
|
||||
bytes: "2.0.0", // global
|
||||
express: {
|
||||
bytes: "1.0.0", // override for express's bytes dependency
|
||||
},
|
||||
},
|
||||
}),
|
||||
);
|
||||
install(tmp, ["install"]);
|
||||
|
||||
// bytes is overridden when required by express
|
||||
expect(versionOf(tmp, "node_modules/bytes/package.json")).toBe("1.0.0");
|
||||
|
||||
ensureLockfileDoesntChangeOnBunI(tmp);
|
||||
});
|
||||
|
||||
test("nested overrides - npm format with only global override in object", async () => {
|
||||
const tmp = tmpdirSync();
|
||||
writeFileSync(
|
||||
join(tmp, "package.json"),
|
||||
JSON.stringify({
|
||||
dependencies: {
|
||||
express: "4.18.2",
|
||||
},
|
||||
overrides: {
|
||||
bytes: {
|
||||
".": "1.0.0",
|
||||
},
|
||||
},
|
||||
}),
|
||||
);
|
||||
install(tmp, ["install"]);
|
||||
|
||||
// Global override via "."
|
||||
// Express depends on bytes, should get the parent-specific override (1.0.0)
|
||||
expect(versionOf(tmp, "node_modules/bytes/package.json")).toBe("1.0.0");
|
||||
|
||||
ensureLockfileDoesntChangeOnBunI(tmp);
|
||||
@@ -397,22 +370,19 @@ test("nested overrides with multiple parents", async () => {
|
||||
"body-parser": "1.20.1",
|
||||
},
|
||||
overrides: {
|
||||
bytes: {
|
||||
express: {
|
||||
bytes: "1.0.0",
|
||||
},
|
||||
"body-parser": {
|
||||
bytes: "2.0.0",
|
||||
},
|
||||
express: {
|
||||
bytes: "1.0.0",
|
||||
},
|
||||
"body-parser": {
|
||||
bytes: "2.0.0",
|
||||
},
|
||||
},
|
||||
}),
|
||||
);
|
||||
install(tmp, ["install"]);
|
||||
|
||||
// Both express and body-parser depend on bytes
|
||||
// Both express and body-parser depend on bytes with different overrides
|
||||
// The actual version will depend on which parent is resolved first
|
||||
// but the override should apply to at least one of them
|
||||
const bytesVersion = versionOf(tmp, "node_modules/bytes/package.json");
|
||||
expect(["1.0.0", "2.0.0"]).toContain(bytesVersion);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user