Skip to content

bake: validate app.bundlerOptions and its subfields are objects #30409

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 1 commit into from
Closed

bake: validate app.bundlerOptions and its subfields are objects #30409

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions src/bake/bake.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,25 @@
}

if (try config.getOptional(global, "bundlerOptions", JSValue)) |js_options| {
if (!js_options.isObject()) {
return global.throwInvalidArguments("'" ++ api_name ++ ".bundlerOptions' must be an object", .{});
}
if (try js_options.getOptional(global, "server", JSValue)) |server_options| {
if (!server_options.isObject()) {
return global.throwInvalidArguments("'" ++ api_name ++ ".bundlerOptions.server' must be an object", .{});
}
bundler_options.server = try BuildConfigSubset.fromJS(global, server_options);
}
if (try js_options.getOptional(global, "client", JSValue)) |client_options| {
if (!client_options.isObject()) {
return global.throwInvalidArguments("'" ++ api_name ++ ".bundlerOptions.client' must be an object", .{});
}
bundler_options.client = try BuildConfigSubset.fromJS(global, client_options);
}
if (try js_options.getOptional(global, "ssr", JSValue)) |ssr_options| {
if (!ssr_options.isObject()) {
return global.throwInvalidArguments("'" ++ api_name ++ ".bundlerOptions.ssr' must be an object", .{});
}
bundler_options.ssr = try BuildConfigSubset.fromJS(global, ssr_options);
}
}
Expand Down Expand Up @@ -215,13 +227,19 @@
}

if (try js_options.getOptional(global, "minify", JSValue)) |minify_options| brk: {
if (minify_options.isBoolean() and minify_options.asBoolean()) {
options.minify_syntax = minify_options.asBoolean();
options.minify_identifiers = minify_options.asBoolean();
options.minify_whitespace = minify_options.asBoolean();
if (minify_options.isBoolean()) {
if (minify_options.asBoolean()) {
options.minify_syntax = true;
options.minify_identifiers = true;
options.minify_whitespace = true;
}
break :brk;
}

Check failure on line 237 in src/bake/bake.zig

View check run for this annotation

Claude / Claude Code Review

minify: false does not disable minification in production 

When `minify` is the boolean `false`, this enters the `isBoolean()` branch, skips the inner `if (asBoolean())` block, and breaks with `minify_syntax`/`minify_identifiers`/`minify_whitespace` still `null` — so in production (`bake.zig:792-794` does `orelse (mode != .development)`) an explicit `minify: false` is silently ignored and minification stays on. This is technically pre-existing release-build behavior, but since this PR is rewriting these exact lines to "fix the minify handling" it's the
Comment on lines +230 to 237

@claude claude Bot May 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 When minify is the boolean false, this enters the isBoolean() branch, skips the inner if (asBoolean()) block, and breaks with minify_syntax/minify_identifiers/minify_whitespace still null — so in production (bake.zig:792-794 does orelse (mode != .development)) an explicit minify: false is silently ignored and minification stays on. This is technically pre-existing release-build behavior, but since this PR is rewriting these exact lines to "fix the minify handling" it's the right place to also fix the semantic: drop the inner guard and assign minify_options.asBoolean() to all three fields (matching JSBundler.zig:694-698).

Extended reasoning...

What the bug is

BuildConfigSubset.fromJS parses app.bundlerOptions.{server,client,ssr}.minify. After this PR the boolean path is:

if (minify_options.isBoolean()) {
    if (minify_options.asBoolean()) {
        options.minify_syntax = true;
        options.minify_identifiers = true;
        options.minify_whitespace = true;
    }
    break :brk;
}

When the user passes minify: false, isBoolean() is true so we enter the outer branch, but asBoolean() is false so we skip the assignments and break :brk. The three ?bool fields on BuildConfigSubset keep their default value of null.

Code path that triggers it

  1. production.zig:238-241 calls framework.initTranspilerWithOptions(..., .production_static, ..., options.bundler_options.server.minify_whitespace, options.bundler_options.server.minify_syntax, options.bundler_options.server.minify_identifiers) (and likewise for client / ssr).
  2. In bake.zig:792-794: 
    out.options.minify_syntax = minify_syntax orelse (mode != .development);
out.options.minify_identifiers = minify_identifiers orelse (mode != .development);
out.options.minify_whitespace = minify_whitespace orelse (mode != .development);
    With mode = .production_static, null orelse (mode != .development) evaluates to true.

So app.bundlerOptions.server.minify = false (and client/ssr) produces a fully-minified production build.

Why existing code doesn't prevent it

Nothing else writes these fields. The null → default fallback in initTranspilerWithOptions is intentional for the unset case, but here the user explicitly set a value and we dropped it. Note that the object form works correctly — minify: { whitespace: false, syntax: false, identifiers: false } reaches the getBooleanLoose calls and sets the fields to false, so the boolean shorthand is now inconsistent with the long form.

Step-by-step proof

Config:

Bun.serve({ app: { bundlerOptions: { client: { minify: false } }, framework: ... } });
  • fromJS sees minify_options = JS false.
  • minify_options.isBoolean() → true, enter branch.
  • minify_options.asBoolean() → false, skip inner block.
  • break :brk → options.minify_whitespace == null, options.minify_syntax == null, options.minify_identifiers == null.
  • Production build: production.zig:239 passes those three nulls into initTranspilerWithOptions.
  • bake.zig:792-794: null orelse (.production_static != .development) → null orelse true → true.
  • Result: client transpiler has minify_syntax = minify_identifiers = minify_whitespace = true. The user's minify: false was a no-op.

Impact

Users who try to disable minification for a Bake production build via the boolean shorthand get minified output anyway, with no error or warning. The only escape hatches are the object form or --debug-disable-minify (production.zig:244-248). This is surprising because every other minify parser in the repo honors false.

Relationship to this PR / pre-existing behavior

In release builds the old code had the same net effect: the old condition was if (minify_options.isBoolean() and minify_options.asBoolean()), which was false for minify: false, and execution then fell through to getBooleanLoose(global, "whitespace") on a boolean primitive — false.whitespace is undefined, so the fields stayed null. So the semantic bug pre-dates this PR.

However, this PR explicitly restructures these exact lines and the description says it "fixes the minify handling to correctly break out of the block when minify: false is passed." It fixes the debug assert (good) but preserves the semantic no-op. Since the author is already rewriting this hunk and the correct behavior is well-established elsewhere, this is the right PR to fix it rather than leave minify: false half-broken.

Fix

Drop the inner guard and assign the boolean to all three fields, matching JSBundler.zig:694-698, JSTranspiler.zig, and bunfig.zig:

if (minify_options.isBoolean()) {
    const value = minify_options.asBoolean();
    options.minify_syntax = value;
    options.minify_identifiers = value;
    options.minify_whitespace = value;
    break :brk;
}

(Equivalently, add an else that sets all three to false.)


if (!minify_options.isObject()) {
return bun.jsc.Node.validators.throwErrInvalidArgType(global, "minify", .{}, "boolean | object", minify_options);
}

if (try minify_options.getBooleanLoose(global, "whitespace")) |value| {
options.minify_whitespace = value;
}
Expand Down
20 changes: 20 additions & 0 deletions test/js/bun/http/bun-serve-args.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,3 +670,23 @@ describe("Bun.serve unix socket validation", () => {
}
});
});

describe("app.bundlerOptions validation", () => {
test("non-object bundlerOptions throws", () => {
expect(() => serve({ port: 0, app: { bundlerOptions: 551 } } as any)).toThrow(
"'app.bundlerOptions' must be an object",
);
});

test.each(["server", "client", "ssr"] as const)("non-object bundlerOptions.%s throws", key => {
expect(() => serve({ port: 0, app: { bundlerOptions: { [key]: 551 } } } as any)).toThrow(
`'app.bundlerOptions.${key}' must be an object`,
);
});

test("non-object non-boolean minify throws", () => {
expect(() => serve({ port: 0, app: { bundlerOptions: { server: { minify: 551 } } } } as any)).toThrow(
'The "minify" property must be of type boolean | object',
);
});
});
Loading