Repository navigation
Conversation
…a.url) bindings
When an ESM module creates its own require with
const req = createRequire(import.meta.url), calls like req("./data.json")
resolve relative to that source file, but the bundler copied them through
verbatim. In the output, import.meta.url points at the bundle, so the
lookup fails at runtime once the bundle is moved away from node_modules
(css-tree inside playwright hits this with ../data/patch.json).
Track createRequire imported from "module"/"node:module" at parse time,
record const bindings initialized with createRequire(import.meta.url)
during the visit pass, and rewrite calls on those bindings with a static
string argument through the same transpose_require path as require().
Dynamic arguments and non-const bindings are left untouched.
Fixes #36564
|
Warning Review limit reached
Next review available in: 29 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Updated 9:39 AM PT - Jul 31st, 2026
❌ @robobun, your commit ed96eec has some failures in 🧪 To try this PR locally: bunx bun-pr 36565That installs a local version of the PR into your bun-36565 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
…meta.url With CJS output format or Bake, the visit pass inlines import.meta.url to a string literal, so checking the argument after visiting never matched and createRequire bindings went untracked. Capture the argument shape before visiting; the callee ref is still checked after, since refs are only resolved during the visit.
There was a problem hiding this comment.
The CJS-format ordering issue I flagged earlier is fixed in b56382f (arg shape captured pre-visit, callee ref checked post-visit) and locked in by the new CreateRequireImportMetaUrlCjsFormat test. No further issues found, but this is a new bundler semantic in the parser visit pass, so worth a maintainer look.
What was reviewed:
- Verified the pre-visit/post-visit split in
visit_declsnow survivesmaybe_rewrite_property_accessinlining under--format=cjs. - Checked the
e_callhook sits after arg/target visiting and mirrors the adjacentERequireCallTarget→transpose_requirepath; onlyEIdentifiertargets with a singleEStringarg match, soreq.resolve(...)and dynamic args fall through unchanged. - Confirmed the
const-only gate plus per-symbolRefmatching prevents shadowed/reassigned bindings from being picked up.
Extended reasoning...
Overview
Adds bundler support for the const req = createRequire(import.meta.url); req("./x") pattern (used by css-tree). Two new Ref/RefMap fields on P, a parse-time hook recording the createRequire import binding, a visit_decls hook recording qualifying const bindings, and an e_call hook routing static-string calls on those bindings through the existing transpose_require path. Four new itBundled tests.
Security risks
None. Bundler-only, no runtime API surface, no untrusted-input parsing beyond what the JS parser already does.
Level of scrutiny
Moderate-high. visit_decls and e_call are hot, subtle parser visit-pass code where ordering matters (my earlier review caught an ordering bug here). The change is a new bundler semantic — deciding to statically bundle something that was previously a runtime call — so a maintainer should sanity-check that transpose_require is the right entry point and that this matches esbuild's behavior for the same pattern.
Other factors
- My previous review found a real ordering bug (arg shape checked post-visit, defeated by CJS-format inlining); it was fixed exactly as suggested and a regression test added. I re-traced the fix and it's correct.
- The change is fully additive and gated: only fires when bundling, only for
constbindings, only for single-EStringargs, only when the callee ref matches the recordedcreateRequireimport. All other shapes fall through to existing behavior. ignore_usage(target_ref)on the transformed call mirrors how the regular require path handles usage counting.- Tests place the entry in
/srcwith output at/out.jsso an unbundled runtime lookup can't accidentally resolve — good fails-for-the-right-reason design. Coverage: ESM, CJS format, aliased import fromnode:modulewithimport.meta.filename, and dynamic-arg fallthrough. - Minor limitation (not blocking): a second
import { createRequire as x } from ...overwritescreate_require_ref, and calls inside a hoisted function declared before the const won't be picked up. Both are edge cases with no regression from current behavior. - The comment-cop threads are all resolved; remaining comments are short field docs and one ordering note that earned its keep.
… fixed at build instead of read from /$bunfs @achingbrain/ssdp (pulled in by nat-port-mapper) reads its own package.json through createRequire(import.meta.url); Bun's bundler leaves that require to run time (oven-sh/bun#36565), where it resolves against /$bunfs and kills `cawco up` at start. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Closing this PR because it has been inactive for more than 90 days. |
|
Rebased onto main and reopened as #47423 (GitHub does not allow reopening a PR after a force-push). The new PR also covers the review findings from here: multiple createRequire imports in one file, constant folding and ternary arguments, req.resolve, and tree-shaking of the dead binding. |
Fixes #36564
Problem
requirein ESM (css-tree, which playwright pulls in) loads JSON throughconst require = createRequire(import.meta.url); require('../data/patch.json'). The bundler copied the call through verbatim. In the output,import.meta.urlpoints at the bundle, so the lookup fails once the bundle moves away fromnode_modules:error: Cannot find module '../data/patch.json' from '/path/to/output/index.js'.--reject-unresolveddoes not fire either, because the callee is a user binding and never gets classified asrequire.Fix
process_import_statement(src/js_parser/p.rs) records the local bindings ofcreateRequireimported frommodule/node:modulewhen bundling.visit_decls(src/js_parser/visit/mod.rs) recordsconstbindings initialized withcreateRequire(import.meta.url | import.meta.filename). The argument shape is checked before the visit, because CJS output inlinesimport.meta.urlto a string during it. The call is marked unwrappable-if-unused so the binding and its import tree-shake once every call is bundled.e_call(src/js_parser/visit/visit_expr.rs) routes calls on those bindings through the same paths asrequire(): string literals go totranspose_require, ternaries split into two requires,req.resolve("...")goes totranspose_require_resolve_known_string, and the forced constant folding thatrequire()gets applies too. Dynamic arguments stay runtime calls on the binding.test/bundler/bundler_edgecase.test.ts(entry in/src, output at/out.js, so an untraced lookup cannot resolve by accident). Six fail on bun 1.4.3 withCannot find module. Also ranbundler_edgecase,bundler_cjs2esm,bundler_bun,bundler_minify,bundler_npm,esbuild/default, andcompile/Bun.embeddedFiles.Background
require("x")in the parser: therequireidentifier becomesERequireCallTarget, ande_callturns a string-literal call intoE::RequireStringwith an import record. That import record is what makes the bundler follow the file.createRequire(import.meta.url)returns a function that resolves relative to the file whose URL it was given. Inside a bundle that file is the source module, so the static calls mean the same thing asrequire()from that module.EStringarguments: that diverged fromrequire()forreq('./a' + '.json'), which only bundled under--minify-syntax. The fix reuses therequire()transpose helpers so the two paths cannot drift.Downsides
node_modules, sees the build-time copy instead.createRequire: oneis_empty()check on a hash map ine_calland one invisit_decls. No allocation, no import record.constbindings (let req = createRequire(...)) and bindings created inside functions after a call site in a hoisted function are still left as runtime lookups.Notes
createRequireimport refs instead of one slot (two imports in one file), forced folding and ternary transposition,req.resolve, and tree-shaking of the dead binding.req.resolverecords__requireusage the same way an uncalledrequireidentifier does, because the printer emits__require.resolve(...)forE::RequireResolveStringon non-bun targets.createRequirebug) #13076 and [css-tree] error: Cannot find module "../data/patch.json" from "undefined" (may be createRequire bug) claude trigger #30546 are the runtime flavor of this css-tree failure (tsx overridingModule._resolveFilename, no bundler). This change does not affect them.