bake: validate app.bundlerOptions and sub-options are objects#30422
Closed
robobun wants to merge 2 commits into
Closed
bake: validate app.bundlerOptions and sub-options are objects#30422robobun wants to merge 2 commits into
app.bundlerOptions and sub-options are objects#30422robobun wants to merge 2 commits into
Conversation
Previously, passing a non-object value for app.bundlerOptions,
app.bundlerOptions.{server,client,ssr}, or their .minify option would
trigger a debug assertion in JSValue.get() instead of throwing a
proper JS TypeError.
Also fixes minify: false falling through to the object-property path.
Collaborator
Author
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR refines bundler options validation for ChangesBundler Options Validation with Property-Qualified Errors
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/bake/bundler-options-validation.test.ts`:
- Around line 16-54: Three parameterized tests use test.each(...) but must
follow guidelines and be converted to describe.each(...) with inner test(...)
blocks; replace each test.each(["server","client","ssr"] as const)(...) (the
first block that asserts bundlerOptions.%s is an object, the second block that
asserts bundlerOptions.%s.minify type, and the third block labeled "accepts
minify: false without crashing for %s") with describe.each([...])('%s', key => {
test("throws when ...", () => { ... }) }) (preserving the existing expectation
logic and messages and keeping Bun.serve invocations and error assertions
unchanged). Ensure the describe.each label and each inner test name mirrors the
original message so test output stays the same.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 48407c21-cbda-48db-8a1f-03fba20d71c8
📒 Files selected for processing (2)
src/bake/bake.zigtest/bake/bundler-options-validation.test.ts
Comment on lines
+16
to
+54
| test.each(["server", "client", "ssr"] as const)("throws when bundlerOptions.%s is not an object", key => { | ||
| expect(() => Bun.serve({ app: { bundlerOptions: { [key]: 2 } as any } } as any)).toThrow( | ||
| `'app.bundlerOptions.${key}' must be an object`, | ||
| ); | ||
| expect(() => Bun.serve({ app: { bundlerOptions: { [key]: "x" } as any } } as any)).toThrow( | ||
| `'app.bundlerOptions.${key}' must be an object`, | ||
| ); | ||
| expect(() => Bun.serve({ app: { bundlerOptions: { [key]: true } as any } } as any)).toThrow( | ||
| `'app.bundlerOptions.${key}' must be an object`, | ||
| ); | ||
| }); | ||
|
|
||
| test.each(["server", "client", "ssr"] as const)( | ||
| "throws when bundlerOptions.%s.minify is not a boolean or an object", | ||
| key => { | ||
| expect(() => Bun.serve({ app: { bundlerOptions: { [key]: { minify: 5 } } as any } } as any)).toThrow( | ||
| `'app.bundlerOptions.${key}.minify' must be a boolean or an object`, | ||
| ); | ||
| expect(() => Bun.serve({ app: { bundlerOptions: { [key]: { minify: "yes" } } as any } } as any)).toThrow( | ||
| `'app.bundlerOptions.${key}.minify' must be a boolean or an object`, | ||
| ); | ||
| }, | ||
| ); | ||
|
|
||
| test("does not crash with self-referencing bundlerOptions and non-object sub-options", () => { | ||
| const v2: any = {}; | ||
| v2.client = 2; | ||
| v2.bundlerOptions = v2; | ||
| expect(() => Bun.serve({ app: v2 } as any)).toThrow("'app.bundlerOptions.client' must be an object"); | ||
| }); | ||
|
|
||
| test.each(["server", "client", "ssr"] as const)("accepts minify: false without crashing for %s", key => { | ||
| expect(() => Bun.serve({ app: { bundlerOptions: { [key]: { minify: false } } as any } } as any)).toThrow( | ||
| "'app' is missing 'framework'", | ||
| ); | ||
| expect(() => Bun.serve({ app: { bundlerOptions: { [key]: { minify: true } } as any } } as any)).toThrow( | ||
| "'app' is missing 'framework'", | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify parameterized test style usage in this file
rg -nP '\btest\.each\s*\(' test/bake/bundler-options-validation.test.tsRepository: oven-sh/bun
Length of output: 332
Convert test.each(...) blocks to describe.each(...) + inner test(...) for guideline consistency.
Per coding guidelines, parameterized tests must use describe.each(). Convert all three test.each() blocks at lines 16, 28, and 47 to the pattern shown below:
♻️ Example refactor pattern
- test.each(["server", "client", "ssr"] as const)("throws when bundlerOptions.%s is not an object", key => {
- expect(() => Bun.serve({ app: { bundlerOptions: { [key]: 2 } as any } } as any)).toThrow(
- `'app.bundlerOptions.${key}' must be an object`,
- );
- // ...
- });
+ describe.each(["server", "client", "ssr"] as const)("bundlerOptions.%s", key => {
+ test("throws when value is not an object", () => {
+ expect(() => Bun.serve({ app: { bundlerOptions: { [key]: 2 } as any } } as any)).toThrow(
+ `'app.bundlerOptions.${key}' must be an object`,
+ );
+ // ...
+ });
+ });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/bake/bundler-options-validation.test.ts` around lines 16 - 54, Three
parameterized tests use test.each(...) but must follow guidelines and be
converted to describe.each(...) with inner test(...) blocks; replace each
test.each(["server","client","ssr"] as const)(...) (the first block that asserts
bundlerOptions.%s is an object, the second block that asserts
bundlerOptions.%s.minify type, and the third block labeled "accepts minify:
false without crashing for %s") with describe.each([...])('%s', key => {
test("throws when ...", () => { ... }) }) (preserving the existing expectation
logic and messages and keeping Bun.serve invocations and error assertions
unchanged). Ensure the describe.each label and each inner test name mirrors the
original message so test output stays the same.
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
Collaborator
Author
|
Duplicate of #30125 which already has the same fix. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes a debug assertion crash in
Bun.serve({ app: ... })whenapp.bundlerOptions,app.bundlerOptions.{server,client,ssr}, or their.minifyoption is a non-object primitive (number, string, boolean).JSValue.get()asserts that its target is an object, butBuildConfigSubset.fromJSandUserOptions.fromJScalled it on values obtained viagetOptional(..., JSValue)without first checking they were objects.Also fixes
minify: falsefalling through to the object-property read path (which would then assert), since the previous check wasisBoolean() and asBoolean().Repro
Before:
panic(main thread): reached unreachable codeatJSValue.get→bun.debugAssert(target.isObject()).After: throws
TypeError: 'app.bundlerOptions.client' must be an object(etc).How did you verify your code works?
Added
test/bake/bundler-options-validation.test.tscovering all three levels of non-object inputs plus theminify: falsepath.Found by Fuzzilli (fingerprint
9aa9c17813ebf2b4).