Repository navigation
Conversation
|
Updated 7:32 AM PT - Oct 29th, 2026
❌ @robobun, your commit e615d46 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 47320That installs a local version of the PR into your bun-47320 --bun |
|
Reproduced on 1.4.3-canary (367d939) and on a release build of main at 11c3671: printf 'import "./does-not-exist.js";\nclass A { #m() {} f() { this.#m = 1; } }\nexport default A;\n' > warn.js
bun build ./warn.js --outdir=outIt prints only With this branch, both report the warning and the error. CI. The diff is green. Each of the two runs has one red lane, and neither failure comes from this change:
Every other lane passed in both runs, and no lane reports a failure in the two test files of this PR. The PR is ready for a maintainer. PR: #47320 |
…t resolve `run_resolution_for_parse_task` replaces the parse result, and the log in it, with an error when an import does not resolve. `bun build` and `Bun.build` then reported the resolve error alone. The function now moves the parse log to the build log, as the success path does. The statement is in the branch that handles a failed import, and that branch is marked cold.
a40b4d5 to
f810638
Compare
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Append resolver messages to transpiler logs src/bundler/bundle_v2.rs |
On resolver failure, the code marks the path cold and appends ResultError.log messages to the transpiler log before constructing the resolve-step error. |
Verify API and CLI build logs test/bundler/bun-build-api.test.ts, test/bundler/cli.test.ts |
Concurrent build API tests check that returned logs and AggregateError.errors include unresolved-import errors and JavaScript and CSS parse warnings. CLI tests check that both errors and warnings appear, stdout is empty, and the process exits with code 1. |
Suggested reviewers: jarred-sumner, alii
Priority:
Merge Risk: 
Minimal · up to f8106
-
The change preserves parse warnings when imports fail to resolve, with regression coverage for JavaScript and CSS. No actionable merge-blocking issue was identified; mergeability remains subject to normal build and test checks.
Pre-merge checks: 
4
-
Full details: Description check
-
Explanation
The description is complete and covers the change, rationale, limitations, verification steps, test results, and performance impact. It does not use the template headings exactly, but it provides the required information under equivalent sections.
-
Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
There was a problem hiding this comment.
LGTM — a one-statement fix that mirrors the existing success-path call, with tests on both the API and CLI surfaces.
What was reviewed:
- Confirmed
on_parse_task_completesetsprocess_log = falseafter aStep::Resolvefailure, soResultError.logis indeed never read on that path and movingresult.loginto the build log before the overwrite is the only place the messages can survive. Log::append_tomovesmsgsand the counters out of the parse log, so dropping the discardedSuccessresult afterwards cannot double-free or double-count.- Dev-server builds: the success arm already appends parse warnings to the same
transpiler.log, so this only makes the resolve-failure case consistent rather than introducing a new destination. core::hint::cold_pathis already used insrc/semaandbun_core::hint; the cli test sits inside an existingdescribe.concurrent, drains pipes withPromise.all, and asserts one combined{ stdout, diagnostics, exitCode }object.
Extended reasoning...
The change adds one result.log.append_to(this.transpiler.log_mut()) call and a cold_path() hint in the import-resolution failure branch of run_resolution_for_parse_task in src/bundler/bundle_v2.rs, plus two tests (Bun.build API with throw true/false, and the bun build CLI) covering a JS file and a CSS file. It touches no security-sensitive surface: no user-controlled parsing, auth, or path handling changes. The fix is a single statement identical to the one on the success path, the dead-log claim was verified against the caller, no CODEOWNERS entry covers the changed files, and the hunt ran dry with no findings, which decided approve.
Problem
bun build ./warn.jsprints onlyerror: Could not resolve: "./does-not-exist.js", andBun.build().logsholds only theResolveMessage.run_resolution_for_parse_task(src/bundler/bundle_v2.rs:6561) replaces the parse result, and the log in it, with an error that has an empty log.Fix
test/bundler/bun-build-api.test.tsandtest/bundler/cli.test.ts. Both fail without the fix. Notes lists the other suites.Background
Downsides
logs,AggregateError.errorsandonEndgainwarnentries after the resolve errors.on_parse_task_complete16007 B -> 15835 B, and 427 -> 425 instructions per parsed file on a build that succeeds. A failing build with one warning makes about 51 morewritecalls and no more allocations.Notes
Repro
Before: only
error: Could not resolve: "./does-not-exist.js". After:warn: Writing to read-only method "#m" will throw, then the error. The exit code is 1 in both. A stylesheet with@import "./does-not-exist.css"and an@nestrule loses and keeps its warning in the same way.Origin. 81e4203 (2023-05-08, "[bundler] Make module resolution single-threaded") moved resolution to the bundle thread. Since then the resolve failure branch builds the error with
Log::init(). Of the five places that build aResultError, it is the only one that does not carry the parse log.Where main already keeps these warnings. The same file reports them when it has a parse error, when an
onResolveplugin handles the import, and when the warning is in another file of the build. esbuild 0.21.5 prints1 warning and 1 errorforwarn.js. That comparison holds for one file only: esbuild also parses the other imports of a failed file, and bun does not.Order. For one file the log holds the resolve errors, then the parse messages. The CLI prints warnings above errors.
Dev server. Not changed. There the parse log goes to the dev server's build log, like the log of a file that builds, and the dev server does not show that log. A variant that moved the log to the failure log of the file did show the warnings in the error overlay. It is not in this PR: a warning with line 0, or with a line and a column of 0 or -1, breaks the wire writer or the overlay (
write_log_datainserialized_failure.rs,renderCodeLineinoverlay.ts). #39488 has the changes for that.AOT. The
bun build --compile --aotpage bundle takes the same path (src/bundler/aot/mod.rs:144passes no dev server). I read the code. I did not run it.CSS positions. A kept CSS warning has the position that the CSS parser gives every warning today: a 0-based line and no source line. #45787 changes that.
Fail-before and mutation. Both new tests fail on a build without the statement: the
warnentries are missing and nothing else differs.Measurements. Release builds on linux-x64, built in one directory: main at 3a9ea00, and the same commit with the source diff of this PR.
on_parse_task_complete427 -> 425 self instructions per parsed file (200-module graph, 202 calls,qemu-x86_64 -one-insn-per-tb). The new call runs 0 times in that build.on_parse_task_complete16007 B -> 15835 B, text +0 B, data and bss unchanged (nm -Sandsize).warn.js: +0 allocations (3282 in both), about +51writecalls (30 to 32 -> 82 to 83, the text of the printed warning). Build that succeeds: +0 allocations (3517 in both) and +0writecalls (55 in both). Counted with gdb breakpoints on the mimalloc entry points and withqemu-x86_64 -strace, three runs each.Bun.buildlogs for a file with a warning and an unresolved import: 1 -> 2 entries, for JS and for CSS. 0 of 3 control outputs changed (a file with a warning only, a stylesheet with a warning only, a failing file with no warning).bloaty,strace,valgrindandperfare not installed here, so the numbers come fromnm,size, qemu and gdb.Suites run on a debug build. On the current branch:
test/bundler/bun-build-api90 pass,test/bundler/bundler_regressions9, and the newcli.test.tstest with its neighbour. On this branch before its last rebase (base 3a9ea00):test/bundlerbundler_plugin68 pass,bundler_barrel70,bundler_browser23,bundler_files28,esbuild/css68.test/bake/devbundle54,css18,plugins3,html15,harness3.test/js/bun/resolve/resolve-error32,test/js/bun/css/invalid-utf8-column6. Timeouts on the 1.6 GB debug binary, none in the changed branch: three--compiletests incli.test.ts(5 s limit), two inbun-build-api.test.ts(an--aotcompile and the build that runs thousands of times), and the 20000-moduleDeepImportDiamondDAGbuild inbundler_edgecase.test.ts(60 s limit).Self-review. An earlier form of this change also moved the log to the dev server's failure log of the file, with a one-line guard in the wire writer. The review raised 17 concerns. Ten were about that dev server half: the guard was one of three cases of the wire format, the other two could abort the dev server or break the overlay, and the failure log key would go out of step with #47062. This PR has no dev server half now, so those are gone. The other seven are answered in this text. One suggestion is not taken: to land the wire format changes of #39488 first as a separate PR. That is a different bug, and #39488 has it.
History. An earlier form of this statement was commit 971c939 in #41801. That PR was closed for its CSS decoding change and stays closed.
no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bundler/cli.test.ts, test/bundler/bun-build-api.test.ts