Conversation
Bundling a file that references __dirname or __filename with --target=bun
(or --target=node) previously inlined the build machine's absolute source
path as a string literal:
var __dirname = "/home/user/project/src";
This leaked the build environment into the output, broke the bundle as
soon as it was run from any other location, and made builds
non-reproducible.
The bundler now threads the target through to the parser and, for ESM
output, emits `var __dirname = import.meta.dir` / `import.meta.path`
(Bun) or `import.meta.dirname` / `import.meta.filename` (Node) so the
values resolve to the output file's location at runtime. For CJS output
on those targets the declaration is dropped entirely, letting references
fall through to the module wrapper's __dirname/__filename parameters.
Browser and IIFE output are unchanged (no runtime equivalent exists).
Since --compile implies --target=bun this also gives compiled executables
the virtual $bunfs path for __dirname/__filename, matching
import.meta.dir.
Fixes #4216.
WalkthroughChangesParser options now carry the resolved target into bundle-time parsing. Bun and Node bundles use runtime-aware Target-aware dirname and filename bundling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
The bytecode module-info path in postProcessJSChunk derives contains_import_meta from the AST flag rather than the printer, so set it when the post-visit __dirname/__filename lowering introduces an import.meta reference.
|
Status: ready for review Reproduced on Verification:
Self-review concerns addressed: Bake production gated out ( |
|
Checked the four suggested issues against the diff:
Leaving the PR description as is (#4216 and #17188 only); none of the four should be auto-closed by this change. |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Looked at #29066: it is an overlapping earlier PR, not an exact duplicate. It adds a This PR keys off the bundle target instead, so it covers plain |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@src/js_parser/parse/parse_entry.rs`:
- Around line 1062-1163: Extract the shared __dirname/__filename part
construction into a helper reused by this block and the runtime injection near
the existing dirname/filename builder. Have the helper accept the dirname and
filename value expressions, while owning the shared count, declaration indexing,
DeclaredSymbolList, S::Local statement, and PartTag::DirnameFilename setup;
preserve each caller’s distinct value-expression generation and ensure both
paths apply identical declaration metadata.
In `@test/bundler/cli.test.ts`:
- Around line 223-233: Drain and assert all subprocess streams concurrently: in
test/bundler/cli.test.ts lines 223-233, add browserBuild.stderr.text() to the
Promise.all results and assert it does not contain "error:" before checking
browserExit; in test/regression/issue/04216.test.ts lines 30-45 and 89-105, add
build.stdout.text() to each Promise.all alongside the existing stderr and exited
awaits, preserving the combined-result assertions.
In `@test/regression/issue/04216.test.ts`:
- Around line 30-45: Drain the piped stdout from the Bun build spawned in the
regression test by awaiting build.stdout.text() alongside build.stderr.text()
and build.exited. Keep the existing stderr and exit-status handling unchanged
while ensuring the stdout pipe is consumed before the process completes.
- Around line 1-11: Move the __dirname/__filename runtime-resolution cases from
the regression suite into the existing bundler test file, such as cli.test.ts,
because issue `#4216` was never a correct behavior and is not a regression. Remove
the regression-only header explanation; do not retain this test under
test/regression/issue/.
- Around line 89-105: Drain the piped stdout from the Bun.spawn build in the
CommonJS build test by reading build.stdout alongside build.stderr and
build.exited in the Promise.all call. Preserve the existing build command and
exit-status handling.
- Around line 59-66: Update the process setup around Bun.spawn to select
nodeExe() when testing the --target=node bundle, while retaining bunExe() for
other targets. If nodeExe() is unavailable, skip that branch with an explicit
reason; preserve the existing stdout, stderr, and exit-code collection.
🪄 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: 6eeb75d0-dfaf-429b-84d5-4a2b41b846a0
📒 Files selected for processing (5)
src/bundler/ParseTask.rssrc/bundler/transpiler.rssrc/js_parser/parse/parse_entry.rstest/bundler/cli.test.tstest/regression/issue/04216.test.ts
…regression dir - Extract import_meta_dot and inject_dirname_filename_part helpers shared by the bundle-time and runtime lowering paths so declaration metadata stays identical in both - Move the issue 4216 suite into test/bundler/cli.test.ts: the hardcoded path behavior was never correct, so it is not a regression - Run --target=node bundles under a real node binary (skip if absent) - Drain stdout/stderr on every build spawn
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/regression/issue/04216.test.ts:42-45— Several new subprocess spawns set a pipe they never drain: the twobuildspawns intest/regression/issue/04216.test.ts(lines ~42-45 and ~106-108) setstdout: "pipe"but only awaitstderr.text()+exited, and thebrowserBuildspawn intest/bundler/cli.test.ts:223-230setsstderr: "pipe"but only awaitsstdout.text()+exited. REVIEW.md's harness convention requires draining all pipes concurrently — add the missing.text()to eachPromise.all, or use"ignore"for the unused stream.Extended reasoning...
What's happening
Three newly-added
Bun.spawncalls configure a pipe that is never read:test/regression/issue/04216.test.ts— both the ESM test (lines ~30-45) and the CJS test (lines ~93-108) spawnbun build --outfilewithstdout: "pipe"andstderr: "pipe", but the subsequentPromise.allonly contains[build.stderr.text(), build.exited].bun build --outfilewrites its "Bundled N modules in Xms" summary to stdout (the "log case" tests incli.test.tssnapshot exactly this output), so the stdout pipe receives data that is never consumed.test/bundler/cli.test.ts:223-230— the newbrowserBuildspawn setsstderr: "pipe", but thePromise.allonly contains[browserBuild.stdout.text(), browserBuild.exited]. Any warnings or errors written to stderr are silently discarded.Why this violates the harness convention
REVIEW.md is explicit under Tests reviewers reject:
Subprocess tests: drain pipes concurrently.
Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited])— an unread pipe fills the ~64KB OS buffer and deadlocks the child.The convention exists for two reasons: (1) an unread pipe eventually fills the OS pipe buffer (~64KB) and blocks the child on
write(2), deadlocking the test; (2) discarding a piped stream throws away diagnostic output that would explain a failure — if thebrowserBuildexit-code assertion ever fails in CI, whateverbun buildwrote to stderr is gone.Step-by-step trace
Take the ESM build spawn at
04216.test.ts:30-47:Bun.spawnis called withstdout: "pipe", stderr: "pipe".- The child (
bun build --outfile …) writes ~100 bytes of "Bundled 2 modules in Xms\n\n bundle.mjs …" to stdout and exits 0. - The test awaits
Promise.all([build.stderr.text(), build.exited])— stderr is drained, exit is observed, stdout is never touched. - Because ~100 bytes ≪ 64KB, the child does not block; the test passes. But if a future change makes
bun buildchattier on stdout (verbose logging, many modules), this same pattern would hang.
The
browserBuildcase is symmetric with stderr: a two-line browser build won't emit 64KB of stderr, so no deadlock in practice, but any error output is lost.Impact
No concrete failure or flake will occur with the current tiny outputs — this is a harness-convention violation, not a correctness bug. The practical cost is diagnosability: when one of these assertions fails in CI, the discarded stream is exactly what you'd want to see.
Fix
Add the missing pipe to each
Promise.all:// 04216.test.ts (both build spawns) const [, buildStderr, buildExit] = await Promise.all([build.stdout.text(), build.stderr.text(), build.exited]); // cli.test.ts browserBuild const [browserOut, browserErr, browserExit] = await Promise.all([ browserBuild.stdout.text(), browserBuild.stderr.text(), browserBuild.exited, ]); expect(browserErr).not.toContain("error:");
Alternatively, if the stream is intentionally unused, set it to
"ignore"instead of"pipe".
|
Both findings were against an earlier revision:
The bytecode gap in test coverage was real though: 4cedb52 adds |
- Gate the import.meta.dir lowering on framework.is_none() so Bake production (which inlines import.meta.dir as a per-source string) keeps __dirname consistent with it. - fold.rs: for --format=cjs on Bun/Node targets (outside Bake), rewrite import.meta.dir/.dirname/.path/.filename to the wrapper's __dirname/__filename params instead of inlining the build-time path, so they agree with __dirname now that it falls through to the wrapper. - Add itBundled output-shape tests next to ImportMetaMain that pin the per-target property names, and extend the cjs move-and-run test to cover import.meta.dir === __dirname.
|
Addressed both follow-up nits:
Verified locally: the runtime suite (bun and node, ESM and CJS) and the three output-shape tests all pass with the current head. |
Standalone graph keys are forward-slashed (B:/~BUN/root/app.js), but the CJS wrapper computed __dirname by splitting on backslash only, yielding an empty string in compiled executables on Windows. Split on either separator via lastPathSeparatorIndex. Also split the compile test: --bytecode alone implies CJS output, so the previous test never exercised the ESM bytecode import.meta flag. The existing test now passes --format=esm explicitly, and a new test covers the implied-CJS wrapper values with the forward-slashed graph key.
There was a problem hiding this comment.
No bugs found in this pass; all earlier findings (the has_import_meta flag, the --bytecode implied-CJS test gap, and the Windows CJS-wrapper separator) have been addressed. That said, this changes user-visible bundler semantics for __dirname/__filename under --target=bun|node and widens the Windows __dirname separator handling in JSCommonJSModule for every CJS module — worth a maintainer sign-off.
What was reviewed:
parse_entry.rs: the newimport_meta_names/defer_to_cjs_wrapperbranching and the sharedinject_dirname_filename_parthelper preserve the original part shape anddeclared_symbols.fold.rs: the CJSimport.meta.dir/.path→__dirname/__filenamerewrite records usage on the wrapper refs and only fires outside Bake;import.meta.url/.fileintentionally left as-is (noted in the description).PathInlines.h/JSCommonJSModule.cpp:lastPathSeparatorIndexis identical to the oldreverseFind(PLATFORM_SEP)on POSIX and matches Node's both-separator semantics on Windows; siblingreverseFind(PLATFORM_SEP)sites inImportMetaObject.cppare on the ESMimport.metapath and unaffected by this fix.
Extended reasoning...
Overview
This PR threads the bundle target into parser options so __dirname/__filename in bundled output for --target=bun|node resolve at runtime (via import.meta.dir/.path for ESM, or the CJS wrapper params) instead of inlining the build machine's absolute source path. It refactors two near-identical declaration-injection blocks in parse_entry.rs into shared helpers, adds a CJS-target rewrite in fold.rs so user-written import.meta.dir/.path stay consistent with __dirname, and fixes a Windows bug in JSCommonJSModule.cpp where the CJS wrapper's __dirname was derived by splitting on backslash only (breaking forward-slashed standalone-executable keys). Eight files across parser, bundler, C++ bindings, and tests.
Security risks
None identified. No untrusted input parsing, no auth/crypto/permission surface. The path-separator change is a string-splitting refinement on trusted internal module keys.
Level of scrutiny
Moderate-to-high. The core logic is a targeted refactor of an existing lowering pass, and the C++ change is a small, well-scoped helper. But it is a user-visible semantic change to bundler output — every --target=bun|node bundle that references __dirname/__filename now emits different code and produces different runtime values (chunk location instead of per-source path). That's the right fix for #4216, but it's a product decision (e.g., a bundle that previously happened to work because it ran from the build directory will now report a different path). The Windows lastPathSeparatorIndex change also affects every CJS module's __dirname on Windows, not just standalone executables — behaviorally it now matches Node's path.win32.dirname (both separators), which is a correctness improvement, but it is broader than the standalone-executable case that motivated it.
Other factors
- The PR has been through three prior review rounds from me with substantive findings each time (missing
has_import_metaflag,--bytecodedefaulting to CJS so the guard test wasn't exercising the ESM path, weak substring assertions), all addressed with follow-up commits and test coverage. - Test coverage is thorough: end-to-end runtime tests for both targets × both formats (Node-target run under real
node),itBundledoutput-shape locks with word-boundary regexes, a dedicated--compile --bytecode(implied CJS) test that fails without the Windows separator fix, and the browser fallback preserved. All prior review threads are marked resolved. - The
--target=nodeESM lowering emitsimport.meta.dirname/.filename, which requires Node 20.11+; that's noted in the description but is a compatibility floor a maintainer may want to weigh.
Given the scope (parser + bundler + C++ runtime) and the user-visible behavior change, I'm deferring rather than approving.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
No new findings on this revision — all four earlier rounds of feedback are addressed (has_import_meta flag, --format=esm for the bytecode test, word-boundary output-shape assertions, and the comment-cop cleanups). Deferring to a human because this is a user-facing bundler output-semantics change plus a native-side Windows path-separator change applied to all four __dirname derivation sites in JSCommonJSModule.cpp, not just the standalone-executable one.
What was reviewed:
- Traced the new
targetoption throughParseTask/transpilerintoparse_entry.rsandfold.rs; browser/iife and Bake keep the string-literal path, and the CJS defer-to-wrapper branch correctly skips part injection. lastPathSeparatorIndexon non-Windows is byte-identical to the oldreverseFind(PLATFORM_SEP); on Windows it now honors forward slashes, which the new implied-CJS--compile --bytecodetest covers.- Confirmed the extracted
inject_dirname_filename_parthelper preserves the pre-existing decl/declared_symbols shape and part tag for both call sites.
Extended reasoning...
Overview
The PR threads the bundle target into parser options and uses it in two places: (1) parse_entry.rs post-visit, where __dirname/__filename now lower to import.meta.dir/.path (bun) or import.meta.dirname/.filename (node) for ESM output, defer to the CJS wrapper for CJS output, and keep the build-time string literal for browser/iife/Bake; and (2) fold.rs, where user-written import.meta.dir/.dirname/.path/.filename in CJS output on bun/node targets rewrite to the wrapper's __dirname/__filename refs instead of inlining. The two duplicated part-injection blocks in parse_entry.rs are collapsed into shared import_meta_dot / inject_dirname_filename_part helpers. PathInlines.h gains lastPathSeparatorIndex (both separators on Windows), replacing four reverseFind(PLATFORM_SEP) sites in JSCommonJSModule.cpp so the CJS wrapper's __dirname is derived correctly from forward-slashed standalone-graph keys. Tests: three itBundled output-shape locks in bundler_edgecase.test.ts and a full end-to-end suite in cli.test.ts (build → relocate → run under both Bun and real Node, ESM and CJS, plus --compile --bytecode in both formats).
Security risks
None identified. No new untrusted-input parsing, no auth/crypto/permissions surface. The lastPathSeparatorIndex helper only widens which separator is recognized when deriving __dirname from an already-resolved module key; it does not introduce a new path-traversal surface.
Level of scrutiny
High. This is a deliberate change to bundler output semantics: every existing bun build --target=bun|node consumer whose code references __dirname/__filename will now see the output chunk's runtime location instead of the source file's build-time path. That is almost certainly what users want (#4216, #17188), and it matches what the non-bundling runtime transpiler already does — but it is still a behavior change a maintainer should sign off on. Separately, the JSCommonJSModule.cpp change applies the both-separator split to all four __dirname derivation sites, not only the standalone-executable path that motivated it; on non-Windows it is a no-op, but on Windows it changes how any forward-slashed require-map key is split. That broadening looks correct but is worth a human glance.
Other factors
The PR has been through four rounds of automated review on earlier revisions (missing has_import_meta flag, import.meta.url/.file scope note, substring-vs-word-boundary assertion, and --bytecode implying CJS which also surfaced the Windows separator bug). All are addressed in the current head, the comment-cop threads are resolved by b412c36, and this run's bug hunt found nothing new. Test coverage is thorough — output-shape locks per target, relocated-bundle runtime checks under both Bun and real Node, and both --compile --bytecode formats — with the author reporting Windows x64 and Linux verification. The one intentionally-deferred sibling (import.meta.url/.file still inline in CJS) is documented in the description.
|
Current CI state for reviewers: the latest run (build 80137) is red only from infrastructure, not the diff. Three build-bun compile jobs (linux x64, linux x64-asan, freebsd x64) timed out before producing a binary, which left the downstream test jobs stuck, and the handful of test annotations are flaky tests that passed on retry on unrelated lanes. The same lanes built fine on the previous run of this branch, where every test lane that ran my changed files was green, including both Windows lanes after the CJS wrapper fix. The diff itself is complete and verified locally on linux and Windows x64; it needs a CI re-run or a maintainer look rather than more code changes. |
|
Consolidated into #29066, which now also carries the pieces from this PR that apply to compiled executables: the The part of this PR that is not carried over is the change to plain Nothing in this area has landed on main in the meantime: this PR's tests ( |
1 participant
What
Bundling a file that references
__dirnameor__filenamewith--target=bun(or--target=node) previously inlined the build machine's absolute source path as a string literal:This leaked the build environment into the output, broke the bundle as soon as it was run from any other location, and made builds non-reproducible.
Fix
The parser options now carry the bundle
target. The post-visit__dirname/__filenamehandling inparse_entry.rsbranches on it:__dirname/__filenamebunesmimport.meta.dir/import.meta.pathnodeesmimport.meta.dirname/import.meta.filename(Node 20.11+)bun,nodecjsbrowser/iife/ bakeSince
--compileimplies--target=bun, compiled executables now get the virtual$bunfspath for__dirname/__filename, matchingimport.meta.dir/.path.For CJS output on those targets, user-written
import.meta.dir/.dirname/.path/.filenameare rewritten to the wrapper's__dirname/__filenameinstead of being inlined as build-time strings, so they stay consistent with the now runtime-resolved__dirname.import.meta.urlandimport.meta.filekeep their existing build-time inlining in CJS output: replacing them needs synthesizednode:url/node:pathcalls rather than an identifier substitution, and that pre-existing behavior is intentionally out of scope here. Bake keeps its per-source-file inlining in both formats.Deferring to the wrapper exposed a Windows bug in
JSCommonJSModule: standalone-executable module keys are forward-slashed (B:/~BUN/root/app.js), but the wrapper's__dirnamewas computed by splitting on backslash only, so compiled CJS executables (--bytecodeimplies CJS) got__dirname === ""on Windows. The split now honors both separators (lastPathSeparatorIndexinPathInlines.h);__filenamestays identical to the require-cache key.After
Why this is the right behavior
The non-bundling runtime transpiler already does exactly this (emits
var __dirname = import.meta.dirfor ESM sources); this change brings bundled output in line. esbuild's--platform=nodeleaves__dirnameunbound in ESM output (resolved by the host runtime); the effect is the same but Bun's// @bunbundles skip retranspilation, so we spell theimport.metaaccess out.A bundle collapses many source files into one output chunk, so there is no meaningful per-source-file
__dirnameanymore; every reference now resolves to the output chunk's directory. That is the only value a relocatable bundle can give.Tests
A new suite in
test/bundler/cli.test.tsbundles to a temp dir, copies the output to an unrelated location, runs it, and asserts__dirname/__filenamereport the runtime location (for both--target=bun/node, both ESM and CJS). The--target=nodebundles run under a realnodebinary so the emittedimport.meta.dirname/filenameare exercised on the runtime they target. Also asserts the build-time path never appears in the output. Fails on main, passes with this change. (The suite lives in the bundler test file, nottest/regression/, since the old behavior was never correct.)Updated the existing "
__dirnameand__filenameare printed correctly" test to expect the$bunfspath for--compile --bytecode --format=esm(ESM bytecode, covering the module-record import.meta flag), and preserved its UTF-8 path-literal coverage via a--target=browserbuild. A separate test covers--compile --bytecodewithout--format(implied CJS), where the wrapper exposes the forward-slashed standalone graph key on Windows; that test fails without theJSCommonJSModuleseparator fix. Both verified on Windows x64 and linux.Fixes #4216
Fixes #17188
no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bundler_edgecase.test.ts test/bundler/cli.test.ts