bake: validate app.bundlerOptions and its subfields are objects#30409
bake: validate app.bundlerOptions and its subfields are objects#30409robobun wants to merge 1 commit into
app.bundlerOptions and its subfields are objects#30409Conversation
Bun.serve({ app: { bundlerOptions: { server: 551 } } }) and similar
non-object values for bundlerOptions / server / client / ssr / minify
would hit a debug assertion in JSValue.get instead of throwing a
TypeError.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR adds runtime type validation to ChangesbundlerOptions Validation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #30125. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (minify_options.isBoolean()) { | ||
| if (minify_options.asBoolean()) { | ||
| options.minify_syntax = true; | ||
| options.minify_identifiers = true; | ||
| options.minify_whitespace = true; | ||
| } | ||
| break :brk; | ||
| } |
There was a problem hiding this comment.
🔴 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
production.zig:238-241callsframework.initTranspilerWithOptions(..., .production_static, ..., options.bundler_options.server.minify_whitespace, options.bundler_options.server.minify_syntax, options.bundler_options.server.minify_identifiers)(and likewise forclient/ssr).- In
bake.zig:792-794:Without.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);
mode = .production_static,null orelse (mode != .development)evaluates totrue.
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: ... } });fromJSseesminify_options= JSfalse.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:239passes those threenulls intoinitTranspilerWithOptions. 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'sminify: falsewas 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.)
1 participant
Fixes a debug assertion failure (
reached unreachable codeinJSValue.get) whenBun.serveis passed anapp.bundlerOptionsconfig wherebundlerOptions,server,client,ssr, orminifyare non-object primitives.Also fixes the
minifyhandling to correctly break out of the block whenminify: falseis passed (previously fell through to property access on a boolean, which happened to work only becausefalse.whitespacereturnsundefinedin JS but would assert in debug builds).Found by Fuzzilli. Fingerprint:
f511e8d983505005