Repository navigation
Conversation
WalkthroughChangesCompile-time glob bundling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 4:21 AM PT - Jul 26th, 2026
❌ @robobun, your commit ea58737 has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35699That installs a local version of the PR into your bun-35699 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
entry.js:1-3— This looks like a scratch file used to manually test dynamicimport()while developing the feature — it self-imports and logs, and nothing insrc/,test/, or the build references it. Pleasegit rm entry.jsbefore merge so it doesn't land at the repo root.Extended reasoning...
What the issue is
The PR adds a new file
entry.jsat the repository root:export const id = "entry"; const v = await import("./entry.js"); console.log("loaded", v.id);
This is a scratch reproduction script — it self-imports via a dynamic
import()and logs the result, which is exactly the kind of one-off you'd write to eyeball what the new glob-bundling logic does with a dynamic specifier. It was staged as part of commitbcb96434("glob-bundle: fall back to runtime require/import on map miss") and left in.Why it doesn't belong
- Nothing references it. Grep across the repo for
entry.jshits only bundler test fixtures that use/entry.jsas a virtual path insideitBundled({ files: { "/entry.js": ... } })— none of them read the file at the checkout root. It is not listed in any build script,Cargo.toml, or test harness. - The repo root is not where fixtures live. Test fixtures go under
test/(specificallytest/bundler/for this feature, per the writing-bundler-tests convention and CLAUDE.md's "Create tests in the right folder"). Bun's repo root contains build config (Cargo.toml,package.json,CMakeLists.txt, etc.), not application entry points. - The PR's own tests already cover this shape.
test/bundler/bundler_glob.test.tsexercises template-literalimport()(glob/ImportTemplateLiteral) and the runtime-fallback path (glob/ExtensionlessFallbackImport), so this file is not standing in for a missing test.
Impact
None functionally — the build, the runtime, and the test suite are all unaffected. It is pure repo clutter: a stray top-level
.jsfile that will show up in every fresh clone and inlsat the root. That is why this is filed as a nit rather than a blocking finding.Step-by-step proof
git log --oneline -- entry.jsshows a single commit,bcb96434, which is one of this PR's two commits — the file did not exist onmain.- The file's contents (
await import("./entry.js")on itself +console.log) have no side effects reachable from any Bun entry point; they only run if someone explicitly executesbun entry.jsfrom the checkout root. rg -l 'entry\.js'across the repo returns only test files that construct in-memory/entry.jsfixtures via theitBundled/expectBundledharness; none dereference<repo>/entry.js.- Therefore removing it cannot break anything, and keeping it serves no purpose.
Fix
git rm entry.js
and amend/push.
- Nothing references it. Grep across the repo for
0e121de to
aaeea8b
Compare
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/bundler/bundler_glob.test.ts`:
- Around line 1-4: Add tests in bundler_glob.test.ts covering generated glob
entries with plugin onResolve handling and external resolution, not only normal
resolution. Extend the existing glob test matrix to exercise both behaviors
across the supported alternate modes, verifying the generated import records
honor each contract.
🪄 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: d335419a-a945-4ff1-bafb-04f5970d7126
📒 Files selected for processing (3)
src/js_parser/p.rstest/bundler/bundler_glob.test.tstest/bundler/html-import-manifest.test.ts
There was a problem hiding this comment.
No bugs found after several rounds of fixes, but this is a new user-facing bundler feature (~350 lines of parser logic, a new __glob runtime helper, and a new bun_glob dep on bun_js_parser) with design choices worth a maintainer's sign-off — notably the parser walking the filesystem directly (with the documented --watch gap) rather than going through the resolver.
What was reviewed:
- Prefix guard now requires a real path component — bare
./,../, and stacked../../all bail (covered byBareDotSlashFallsThrough/BareDotDotSlashFallsThrough). glob_parts_from_exprhas the same stack-overflow guard as the other recursive walkers; rope heads/tails are flattened before reading.TransposeStateis threaded sowith: { type },import_record_tag, andHANDLES_IMPORT_ERRORSreach both the bundled entries and the fallback; the fallback'spparam is registered in the scope so it renames correctly.ALL_SORTED/ALL_SORTED_INDEXinruntime.rsre-derived and match; snapshot hash/debugId updates are the expected consequence ofruntime.jschanging.
Extended reasoning...
Overview
Adds esbuild-style glob bundling for require()/import() with template-literal or +-concat arguments. Touches src/js_parser/p.rs (new handle_glob_pattern, glob_parts_from_expr, walk_glob_for_bundle, wired into transpose_import, transpose_require's fallback arm, and the direct require() call arm in visit_expr.rs), src/runtime.js (new __glob helper), src/ast/runtime.rs (registers __glob in the runtime-imports tables), and adds bun_glob as a workspace dep of bun_js_parser. New 425-line bundler_glob.test.ts with 17 cases plus a manual symlink test; three unrelated snapshot files updated for the runtime.js content-hash change.
Security risks
None identified. The glob walk is anchored to the source file's directory, requires a ./ or ../ prefix with a non-dot/slash character, bails on literal glob metacharacters in template segments, and follows symlinks with the walker's built-in cycle protection. This is build-time filesystem enumeration of the user's own project, not runtime input handling.
Level of scrutiny
High. This is new user-facing bundler behavior that changes what gets pulled into every bundle containing a dynamic relative require/import. It runs in the parser hot path (gated on options.bundle), does direct readdir I/O outside the resolver's DirEntry cache (the PR description flags the resulting --watch gap as a follow-up), and encodes several heuristics (* vs **/* placement, dots-and-slashes-only prefix bail, zero-match fallthrough, runtime fallback on map miss). Those are reasonable choices that match esbuild, but they are API-shaped decisions a maintainer should confirm.
Other factors
The PR has been through extensive iteration — earlier rounds caught and fixed dropped import attributes, unflattened rope segments, Windows path-separator handling, the missing transpose_require wiring, an unregistered fallback param symbol, the bare-../ greedy-glob hole, missing stack-overflow guard, follow_symlinks, and undrained subprocess stdout. All threads are resolved and the current diff looks correct. Test coverage is thorough (positive/negative, require/import, ternary, folded constants, onResolve/--external, symlinks, and the original #13672 repro under --compile). Given the scope and the architectural note about moving the walk behind DirEntryAccessor, deferring to a human for final sign-off.
When bundling, require(`./dir/${x}`) and import(`./dir/${x}`) now glob
./dir/**/* at build time and emit a lookup map of bundled modules,
matching esbuild's behaviour. Previously these calls were left as
unbundled runtime requires, so in a --compile'd binary they resolved
relative to /$bunfs/root and failed with 'Cannot find module'.
Fixes #13672
- __glob(map, fallback) tries fallback(path) on miss, so extensionless or
directory-index specifiers ("./engines/" + "boa") still resolve the
way they did before the matching files were bundled.
- Thread TransposeState into the import() path so {with:{type}} and the
.then().catch() / awaited-in-try error-handling heuristic are applied
to the bundled matches and the fallback.
- Record matched files by their relative specifier, not the globbed
absolute path, so --external patterns and onResolve plugins see them.
…es, glob ternary require branches, register fallback param symbol, trim comments
…le-output snapshots for the __glob runtime helper
…not glob the parent tree
bb83080 to
4daed74
Compare
There was a problem hiding this comment.
No new issues found this pass, but deferring to a human — this is a substantial bundler feature (filesystem globbing inside the parser, a new __glob runtime helper, changed bundling semantics for every dynamic relative require/import) with design tradeoffs the PR itself calls out (the --watch gap, walking via direct readdir rather than the resolver's DirEntryAccessor). Worth a maintainer look at the layering and the fallback semantics before landing.
What was reviewed:
- Prefix guard now requires a non-
./character, so bare./,../, and stacked../../all fall through — verified against the pattern-builder and covered byBareDotSlashFallsThrough/BareDotDotSlashFallsThrough. __globfallback threading:import_options,import_loader/tag, andHANDLES_IMPORT_ERRORSare propagated to both the bundled entries and the runtime fallback; the fallback param is registered ingeneratedso it renames instead of shadowing userp.walk_glob_for_bundlenow follows symlinks (cycle-safe viaGlobWalker), andrelative_platform::<Loose, false>emits POSIX keys on Windows.ALL_SORTED/ALL_SORTED_INDEXreshuffle inruntime.rschecked against the derivation test; snapshot/hash updates are the expected fallout of touchingruntime.js.
Extended reasoning...
Overview
This PR adds esbuild-style glob bundling for template-literal / string-concat require() and import() arguments. The core change is ~310 new lines in src/js_parser/p.rs (handle_glob_pattern, glob_parts_from_expr, walk_glob_for_bundle), wired into three call sites (transpose_import, transpose_require's catch-all arm, and the direct require() visitor arm in visit_expr.rs). It adds a new __glob runtime helper to src/runtime.js, registers it in src/ast/runtime.rs (with the sorted-index tables reshuffled), adds bun_glob as a bun_js_parser dependency, and ships a 425-line test file plus snapshot/hash updates in four existing test files that are downstream of the runtime.js change.
Security risks
The parser now walks the filesystem based on user-written string prefixes. The guard requiring a non-./ character in the literal prefix, the bail on literal glob metacharacters, and the ./ / ../ prefix requirement together bound the walk to a named subdirectory of the source file (or a named sibling of an ancestor), which is the same trust boundary the resolver already operates in. Matched files become ordinary import records that go through the resolver / --external / onResolve, so no new bypass of existing gates. I don't see a security concern beyond what static require("./x") already implies.
Level of scrutiny
High. This changes bundler output for a broad class of previously-passthrough inputs, does I/O inside the parser (a layering choice the PR notes as a compromise), and adds user-visible behavior that people will depend on. The __glob map-miss fallback keeps prior behavior working, so the regression surface is bounded, but the design choices — parser-side walk vs. resolver-side, the --watch invalidation gap, whether to warn on zero matches, fallback vs. hard error — are exactly the kind of thing REVIEW.md flags as needing maintainer agreement rather than bot approval.
Other factors
Over prior review passes on this PR I raised (and the author fixed) the dropped import attributes, rope-head flattening, Windows path separators, the missing ternary/transpose_require hook, the un-scoped fallback param, the bare-../ greedy-bundle, symlink following, and a couple of test-hygiene nits — all threads are resolved and reflected in the current diff. Test coverage is thorough (17 itBundled cases plus a manual symlink spawn test and the original #13672 repro under --compile). The remaining hash/debugId snapshot churn is mechanical. Nothing blocking from a correctness standpoint; deferring purely on scope and design sign-off.
There was a problem hiding this comment.
No new issues found, but this is a substantial new bundler feature (~350 LoC in the parser, a new __glob runtime helper, filesystem I/O during parsing) with user-visible design decisions — the glob heuristics, the runtime-fallback shape, and the acknowledged --watch/DirEntry-cache gap — that warrant a maintainer sign-off.
What was reviewed:
- Prefix guard now requires a non-
./character, closing bare./and../(and stacked../../) greedy-bundle cases. glob_parts_from_exprrecursion is stack-guarded like the other AST walkers; literal glob metacharacters bail.- Symlinks under the wildcard are followed (walker has cycle protection); I/O errors and zero matches fall through unchanged.
__globmap keys always carry a.//../prefix, so plain-object prototype lookups (constructor,__proto__) can't collide.
Extended reasoning...
Overview
This PR adds esbuild-style glob bundling for dynamic require()/import() calls whose argument is a template literal or string concatenation with a relative prefix. It touches: src/js_parser/p.rs (~310 new lines: handle_glob_pattern, glob_parts_from_expr, walk_glob_for_bundle), src/js_parser/visit/visit_expr.rs (one new call site), src/runtime.js (new __glob helper), src/ast/runtime.rs (new runtime-import entry + resorted index tables), src/js_parser/Cargo.toml / Cargo.lock (adds bun_glob dep), a new 425-line test file, and hash/debugId snapshot updates driven by the runtime.js change.
Security risks
Low. The glob walk is anchored under the source file's directory and only fires for .//../-prefixed literals that name at least one real path component, so it cannot be steered by attacker-controlled runtime input (it runs at build time on the build machine's filesystem). The __glob runtime helper does a plain-object property lookup on the computed path, but every key the parser emits — and every value the caller can produce given the required literal prefix — starts with ./ or ../, so it cannot collide with Object.prototype members.
Level of scrutiny
High. This is new user-facing bundler behavior: code that previously fell through to a runtime require() now eagerly bundles every file matching a synthesized glob. That is the intended fix for #13672 and matches esbuild, but it changes output for existing projects and involves several heuristics (when to bail, * vs **/*, symlink following, fallback semantics) plus a documented follow-up (the walk uses a direct readdir rather than the resolver's DirEntry cache, so --watch won't pick up new files in a globbed directory). Those are the kind of API/architecture calls a maintainer should confirm.
Other factors
All earlier review threads (bare-../ guard, stack-overflow guard, symlink following, test assertion style, stdout draining, plugin/external coverage) are resolved and reflected in the current diff. Test coverage is broad (17 itBundled cases + a manual symlink test, including the original #13672 --compile scenario). The ALL_SORTED / ALL_SORTED_INDEX table updates in runtime.rs are consistent and covered by the existing unit test in that file. The snapshot churn is expected given runtime.js changed. Nothing blocking from my side; deferring for maintainer sign-off on the feature design.
|
CI state on ea58737: all test failures are flaky (each passed on retry). The only hard failure is the binary-size check, and it is not from this PR: sibling PRs on current main (for example builds 82126 and 82106) report byte-identical deltas against the stale canary #79916 baseline, so the +~550 KB is drift on main since that canary. Reported separately for triage. The diff itself is ready for review. |
|
Closing in favor of #35680, which implements the same build-time glob lookup (a |
What does this PR do?
Fixes #13672.
When
bun build(including--compile) encounters arequire()orimport()whose argument is a template literal or string concatenation with a relative prefix, it now globs the matching files at build time and bundles them into a lookup map, matching esbuild's behaviour.Before
In a
--compile'd binary this resolved relative to/$bunfs/rootand failed:After
The matching files are bundled and the lookup succeeds regardless of where the binary runs. On a map miss (for example
"./engines/" + "boa"with no extension),__globfalls back to the runtimerequire/import()so those cases resolve exactly as they did before this change.How does it work?
When the parser visits
require(expr)/import(expr)andexpris not a plain string:exprinto alternating literal / wildcard segments. Supports untagged template literals and"prefix" + xchains (the left operand of+must itself be analysable so there is a literal prefix to anchor the glob)../or../and names a real path component (so a bare./+ x or../+ x does not glob-bundle an entire directory tree) and there is at least one wildcard./becomes**/*(recurses into subdirectories); a mid-segment wildcard becomes*(current directory only). Literal glob metacharacters (*,?,[,{) in a template segment bail to the runtime path.allowUnresolvedstill applies and runtime-created files can still be loaded.ImportKind::Require/ImportKind::Dynamicimport record (using the relative specifier, so--externalpatterns andonResolveplugins still match) and emit__glob({ "./rel/path": () => <require>, ... }, <fallback>)(expr). Forimport(), the surrounding{ with: { type } }options and the.then().catch()/ awaited-in-try error-handling flag are threaded through to both the bundled entries and the fallback.__globis a new runtime helper insrc/runtime.js.Why is this the correct fix?
The reported bug is not a resolver quirk: the
./engines/*.jsfiles were never bundled because the specifier was dynamic, so there was nothing for the standalone resolver to find. Making the standalone resolver search the build-machine path would only paper over it on the machine that built the binary. Bundling the matches is what actually makes the executable self-contained, and it is the established behaviour in esbuild that users coming from that ecosystem expect. The runtime fallback on map miss keeps everything that worked before working.Notes
The glob walk runs in the parser using a direct
readdir. Each matched file's import record is the relative specifier, so the bundler's resolver,--externalmatching, andonResolveplugins still see every file. The pattern itself is not exposed to plugins (same as esbuild), and the directory listing is not registered with the resolver'sDirEntrycache, so a file added to a globbed directory during--watchdoes not trigger a rebuild until one of the already-tracked files changes. Moving the walk tobundle_v2::resolve_import_recordsbehindDirEntryAccessorwould close that gap; it is a larger refactor left for a follow-up.Tested by
test/bundler/bundler_glob.test.ts(new, 13 cases): template-literalrequire, string-concatrequire, template-literalimport(), extensionless-key fallback forrequireandimport(),import(..., { with: { type } }), zero-match fallthrough, bare-package ignored,*vs**/*wildcard placement,../prefix, literal-*fallthrough, and the original #13672readdir + require(\./engines/${f}`)` scenario run as a compiled standalone executable.9 of the 13 fail on
main(the negative cases that assert the glob does not fire, and the extensionless fallback cases that exercise pre-existing runtime resolution, pass on both).test/bundler/bundler_allow_unresolved.test.ts,bundler_cjs.test.ts, andbundler_edgecase.test.tsstill pass unchanged.no test proof · iteration 5 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bundler_glob.test.ts