Repository navigation
Conversation
transform(), transformSync(), scan() and scanImports() threw the log as a BuildMessage when it held warnings only. A valid source that produced a warning gave the caller no output. Check log.errors at those four sites, which matches the runtime module loader.
|
Warning Review limit reached
On-demand reviews are free for the next 8 days. After that, they cost $0.25 per reviewed file. Or wait 2 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
|
The automated review hit its rate limit and posted no findings. No change is needed for that. CI is still running on 6af5ccc. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked that the moved expectParseError cases are not weakened — the new Bun.build assertions still check the exact message and now additionally assert level: "warn" and the line number. Also confirmed the self.log.has_any() at JSTranspiler.rs:845 is intentionally left as-is: it sits inside the already-gated error branch and only decides whether to synthesize a BuildMessage from self.err when the log is empty.
Extended reasoning...
The four-line Rust change is mechanical and matches the stated intent — each site swaps warnings + errors > 0 / has_any() for has_errors(). I verified the one remaining has_any() in TransformTask::then is inside the rejection branch and governs error-message construction, not the throw gate, so leaving it is correct. On the test side, REVIEW.md flags silent weakening of existing assertions; the six expectParseError calls that relied on the old throw are replaced by a Bun.build check that asserts the same message text plus level and position.line, and a transformSync check that output is still produced — coverage is preserved or stronger. tempDir and join are already imported at the top of the file. Nothing further to add beyond the inline nit about the async transform path in the mixed-error case.
|
Addressed the one suggestion: the warning-plus-error test now also covers the async transform() path, in a53ef51. |
There was a problem hiding this comment.
LGTM — the follow-up commit adds the async transform rejection assertion, so all four changed entry points are now covered for both the warning-only and warning+error cases.
What was reviewed:
- The four
has_any()/warnings + errors > 0→has_errors()swaps inJSTranspiler.rs; constructor-side config checks are untouched as stated. - The reworked private-accessor assertions: coverage moves from
expectParseErrortoBun.buildlogs asserting exact[level, line, message], which is at least as strong. - Test hermeticity:
tempDir+Bun.build({ write: false }), no network, no ports.
Extended reasoning...
Overview
Four call sites in src/runtime/api/JSTranspiler.rs (async TransformTask::then, transformSync, scan, scanImports) switch their throw/reject gate from "any log message" to log.has_errors(), so parser warnings no longer fail the operation. test/bundler/transpiler/transpiler.test.js gains a describe block exercising all four entry points with warning-only input, plus a mixed warning+error case that asserts the AggregateError still surfaces both messages on every entry point (the async path was added in the follow-up commit, addressing the only nit from the earlier review). Six existing expectParseError assertions for private-accessor/method warnings are re-homed into a dedicated test that checks transformSync succeeds and that Bun.build reports each as a warn log with the expected line and message.
Security risks
None. This is a diagnostic-severity gate change in a JS-facing transpiler API; no auth, crypto, network, filesystem, or untrusted-input parsing surface is touched. The parser itself is unchanged — only whether a populated warning log short-circuits into a throw.
Level of scrutiny
Low-to-moderate. The Rust diff is four one-token condition changes with no control-flow restructuring, and the new gate matches the runtime module loader's existing errors > 0 behavior. It is a user-visible behavior change (calls that previously threw now succeed), but the PR description cites a maintainer having proposed exactly this fix in an earlier review, and the constructor-side config checks are deliberately left on the stricter condition. CODEOWNERS does not cover these paths.
Other factors
REVIEW.md's "fix the whole class" and "cover the variant matrix" are satisfied: all four sibling entry points get the same guard and matching tests, and both states (warning-only succeeds; warning+error still throws with the full AggregateError) are asserted. The migrated private-accessor assertions are justified in the PR body and replaced with equal-or-stronger coverage (exact level/line/message via Bun.build logs), so no existing test is silently weakened. The new tests use tempDir from harness and Bun.build({ write: false }), keeping them hermetic.
|
Updated 1:38 AM PT - Sep 12th, 2026
❌ @robobun, your commit 242d4e0 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 42433That installs a local version of the PR into your bun-42433 --bun |
Problem
Bun.Transpilerthrows, or rejects, when the parser logs a warning. The thrown value is aBuildMessagewithlevel: "warn", for exampleTreating "-->" as the start of a legacy HTML single-line comment. The caller gets no output for a source thatbun buildtranspiles with exit 0.src/runtime/api/JSTranspiler.rs:scan(line 1320),transform_sync(line 1504),scan_imports(line 1714) testwarnings + errors > 0, and the asynctransformtestslog.has_any()(line 842).logLevel: "error"hides the warning fortransformandtransformSynconly.scanandscanImportsignore it (Bun.Transpiler: scan() and scanImports() ignore the logLevel option #42426), so no workaround exists for them.Fix
log.has_errors(). A warning-only parse returns the normal result. An error still throws the log as before.errors > 0only (src/runtime/jsc_hooks.rs:2755). A channel that returns warnings would be a new API and is out of scope.errors > 0change in the review of fix: jsx key after spread #7464 (fix: jsx key after spread #7464 (comment)). ThelogLeveloption landed instead because that diff missed the async site.test/bundler/transpiler/transpiler.test.js. The new block "a parser warning does not throw" fails on main (4 of 5 cases) and passes with the fix. The "class static blocks" test asserted six private accessor warnings through the throw. Those cases move to a new test that checks the warnings throughBun.buildlogs. Also ran all oftest/bundler/transpiler/.Background
Log(src/ast/lib.rs). Each message has a kind.warningsanderrorscount them by kind.has_any()iswarnings + errors > 0,has_errors()iserrors > 0.Log::to_jsturns the log into oneBuildMessage, or anAggregateErrorof them when it holds more than one message. With a warning next to an error the thrown value is still thatAggregateError.defineerrors (lines 966, 985, 1021) keep the old condition. They report config problems, not parser diagnostics.Fixes #42427
Related: #7328, #42426, #42430
Notes
-->legacy HTML comment at the start of a line,<div {...obj} key="k" />,<div key />, and the private accessor warnings such asWriting to getter-only property "#x" will throw.scanImportscase in the new test uses the JSX key-after-spread warning. That warning makes the file fall back to the classic runtime, so the result also listsreact/jsx-dev-runtimeandreactasrequire-callimports.test/bundler/transpiler/jsx-production.test.tstimed out in my container when run with the whole directory (32 concurrent spawns, 3.3 s each, 5 s limit). It usesbun run, notBun.Transpiler, and passes when run alone.[human-review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file
root cause · written by the author bot
The four
Bun.Transpilerentry points (transform,transformSync,scan,scanImports) insrc/runtime/api/JSTranspiler.rsthrew the parser log whenever it contained any message, so a mere warning such as the legacy HTML comment notice or the JSX key-after-spread deprecation caused valid source to fail with aBuildMessageof levelwarnand no output. The fix changes each of those guards from a "warnings plus errors" orhas_any()check tohas_errors(), matching the runtime module loader, so warnings no longer abort the call while genuine errors still surface as an `AggregateEr…