Conversation
The module synthesized for an embedded sqlite database called import.meta.require(). With --format=cjs (which --bytecode defaults to) the chunk is wrapped in the @bun-cjs function wrapper, where import.meta is a SyntaxError, so compiled executables failed at startup and bytecode generation failed. The synthesized AST never runs through the visit pass, so nothing rewrote it. For cjs output, call the wrapper's require parameter instead, the same way the napi loader already does.
|
Warning Review limit reached
Next review available in: 3 minutes 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 (3)
Comment |
|
Reproduced on the released 1.4.0 build and on main ( Fix is in this PR. New tests: |
There was a problem hiding this comment.
LGTM — small, well-reasoned fix that swaps import.meta.require for ERequireCallTarget in the sqlite lazy-export AST when output format is cjs, matching the napi loader's existing pattern. The one inline nit (non-embedded type: "sqlite" cjs coverage) is worth adding but non-blocking since both loaders share the same match arm.
Extended reasoning...
Overview
This PR fixes a SyntaxError when bundling embedded sqlite modules with --format=cjs (or --bytecode, which defaults to cjs). The bundler synthesizes a lazy-export AST for the sqlite loader that hard-codes import.meta.require(...), but cjs output for target bun is wrapped in the @bun-cjs function wrapper where import.meta is a SyntaxError. Since lazy-export ASTs skip the visit pass, nothing rewrites it. The fix branches on opts.output_format == Cjs at the AST construction site to emit ERequireCallTarget instead, which the printer renders as bare require (binding to the wrapper's parameter). Three files touched: ~20 lines in ParseTask.rs, plus ~70 lines of new tests in bundler_bun.test.ts and bundler_compile.test.ts.
Security risks
None. This is bundler AST construction for a synthesized module — no user input parsing, no allocation-size arithmetic, no auth/crypto/permissions.
Level of scrutiny
Low-to-medium. The change is narrow (one branch in one match arm), follows the neighboring napi loader's existing pattern (ParseTask.rs:1118 already uses ERequireCallTarget), and only affects a code path that was previously broken (threw SyntaxError at runtime). I verified the printer behavior at js_printer/lib.rs:3288-3299 — ERequireCallTarget prints as bare require when require_ref is None, which the PR description confirms is the case for cjs. I also checked why the fix is cjs-only: the __require linking check in generateCodeForLazyExport.rs:399 matches on the outermost ECall, but sqlite's root is E::Dot, so using ERequireCallTarget in esm/iife would print __require without linking its definition — the PR description states this explicitly and the code confirms it.
Other factors
Test coverage is solid: four new tests (one bun build cjs test with onAfterBundle output-shape assertions, three --compile variants including bytecode with a cache-hit check that fails while bytecode generation fails). The pre-existing esm test is preserved as the first entry in the new loop. The PR description confirms all four fail on USE_SYSTEM_BUN=1 and pass on the debug build. The one bug-hunter finding is a coverage nit (non-embedded Loader::Sqlite shares the arm at line 945 but has no cjs test) — valid, but the code path is literally identical after path_to_use is set, so there is no distinct logic to regress independently. Not blocking.
|
Updated 3:36 PM PT - Aug 13th, 2026
✅ @robobun, your commit e17a37d35c4ca8e5a78f85c4ca99f9734e67d791 passed in 🧪 To try this PR locally: bunx bun-pr 38173That installs a local version of the PR into your bun-38173 --bun |
…embedded sqlite in cjs output
There was a problem hiding this comment.
LGTM — earlier feedback (the bun/sqlite-file-cjs sibling and the comment length) is addressed, and no new issues were found on this pass.
What was reviewed:
- The cjs branch builds
ERequireCallTargetexactly as the napi loader does atParseTask.rs:1114;opts.output_formatis the same field read elsewhere in the file. - Confirmed the pre-existing esm
compile/EmbeddedSqlitecase is preserved as the{ suffix: "", options: {} }variant, and the bytecode variant asserts a disk-cache hit so it fails if bytecode generation regresses. - Checked the
onAfterBundleregexes against the two linker shapes (hoistedvar db_default = require(...)andmodule.exports = require(...)inside a__commonJSwrapper) — both anchor on the barerequireidentifier and rule outimport.meta.
Extended reasoning...
Overview
The PR changes one branch in src/bundler/ParseTask.rs (Loader::SqliteEmbedded | Loader::Sqlite in get_ast): when the output format is cjs, the synthesized lazy-export module calls ERequireCallTarget instead of import.meta.require. The rest of the diff is tests: three new itBundled cases in bundler_bun.test.ts (embedded-cjs, embedded-cjs dynamic import, non-embedded-cjs) and a four-variant matrix replacing the single compile/EmbeddedSqlite test in bundler_compile.test.ts (esm, +cjs, +cjs+minify, +bytecode).
Security risks
None. This is codegen for a bundler-synthesized module; no untrusted input parsing, no auth/crypto, no memory management. The only behavioral change is which identifier the printer emits for the require call target.
Level of scrutiny
Medium — the bundler is a critical path, but this change is a ~15-line format-gated conditional that mirrors the napi loader immediately below it (ParseTask.rs:1112-1119), which already uses ERequireCallTarget unconditionally for the same lazy-export mechanism. The PR description explains why esm/iife keep import.meta.require (a lazy export's ERequireCallTarget would print __require without linking its definition), and that reasoning is consistent with how require_ref is set per format in LinkerContext.rs.
Other factors
Since my previous review, the author addressed both open threads: bun/sqlite-file-cjs was added (my nit), and the multi-line code comment was cut to one line (comment-cop, twice). All inline threads are resolved. Test coverage is thorough: both linker shapes for the lazy export (hoisted var and __commonJS-wrapped module.exports), both loaders (embedded and non-embedded), the compile matrix including identifier minification and bytecode with a disk-cache-hit assertion, and the pre-existing esm cases are preserved. The PR description records that the new tests fail under USE_SYSTEM_BUN=1 and that esm/iife/--bytecode --format=esm were manually re-checked. The bug hunter found nothing on this pass.
|
Heads-up from #39715: that PR makes cjs output for the bun target pass the real |
Problem
import db from "./db.sqlite" with { type: "sqlite", embed: "true" }) fails at startup when built with--compile --format=cjs, or with--compile --bytecode(which defaults to cjs):SyntaxError: import.meta is only valid inside modules.The--bytecodebuild also printserror: Failed to generate bytecode for ./entry.js. Plainbun build --target=bun --format=cjsoutput fails the same way when run.import.meta.require(<embedded path>, { type: "sqlite" }).db(src/bundler/ParseTask.rs, sqlite branch ofget_ast). It is built directly as an AST and handed tonew_lazy_export_ast, which does not run the visit pass, so nothing rewrites theimport.metaand the printer emits it verbatim.// @bun @bun-cjs\n(function(exports, require, module, __filename, __dirname) {(src/bundler/linker_context/postProcessJSChunk.rs), andimport.metainside a function body is a SyntaxError.Fix
ERequireCallTargetinstead ofimport.meta.require, so the module prints asrequire(<embedded path>, { type: "sqlite" }).db. Other formats keepimport.meta.require.ERequireCallTargetas a barerequirefor cjs output (require_refisNonethere,src/bundler/LinkerContext.rs), which binds to the wrapper'srequireparameter: the module's own CommonJS require, the same functionimport.meta.requirereturns, and it accepts the{ type }attributes object as its second argument. The napi loader a few lines below already builds its lazy export this way.// @bun), whereimport.meta.requireworks today, and for a lazy exportERequireCallTargetwould print the runtime's__requirewithout linking in its definition.bun bd test test/bundler/bundler_bun.test.tsandbun bd test test/bundler/bundler_compile.test.ts -t EmbeddedSqlite:bun/embedded-sqlite-file-cjschecks the printed output (@bun-cjswrapper,var db_default = require(...), noimport.meta) and runs it.bun/embedded-sqlite-file-cjs-dynamic-importdoes the same for the other shape the linker gives a lazy export (module.exports = require(...)inside a__commonJSwrapper), which a dynamic import produces.compile/EmbeddedSqlite+cjs,+cjs+minify(syntax, whitespace and identifiers), and+bytecoderun the compiled executable; the bytecode variant also checks that the bytecode cache is hit, which fails while bytecode generation fails.bun/sqlite-file-cjscovers the non-embedded loader (type: "sqlite"withoutembed) in cjs output. That path keeps the import external, so it never emittedimport.meta; the test locks that shape in and runs the bundle.USE_SYSTEM_BUN=1) with the SyntaxError above; the pre-existing esm tests (bun/embedded-sqlite-file,compile/EmbeddedSqlite) pass before and after.--compile --bytecode --format=esmoutput for the same entry still runs.Background
new_lazy_export_ast. Such ASTs skip the parser's visit pass, so format-dependent rewrites that normally happen there have to be made when the expression is built.ERequireCallTarget: the AST node for therequirefunction itself. The printer emits it as the runtime's__requirehelper in esm/iife output and as the bare identifierrequirein cjs output.@bun-cjswrapper: cjs output for target bun is emitted as a function taking(exports, require, module, __filename, __dirname); Bun calls it with the module's CommonJS require, which is also what resolves embedded$bunfspaths in a compiled executable.--bytecodeuses this format by default because bytecode is generated for a CommonJS program.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/bundler_compile.test.ts