Provide a better-sqlite3 module backed by bun:sqlite - #36712
Conversation
better-sqlite3 is a V8-API native addon, so its compiled .node file cannot be loaded by Bun (dlopen throws ERR_DLOPEN_FAILED, see #4290). Its install script also falls back to node-gyp when no prebuilt matches Bun's reported Node ABI, which either compiles a useless binary for minutes or fails outright against newer Node headers. This adds a thirdparty override: require('better-sqlite3') resolves to a shim that wraps bun:sqlite with the better-sqlite3 Database/Statement API (including chainable .raw/.pluck/.bind, .pragma, .columns, and the .deferred/.immediate/.exclusive transaction variants). Packages that depend on better-sqlite3 (drizzle-kit, drizzle-orm, Kysely, knex) now work without a native compile step. Also: - Drop better-sqlite3 from the default-trusted list so bun install no longer runs its install script by default. - Fix the internal-module scanner so builtins can require('bun:sqlite') (the 'bun/' directory was never mapped to the 'bun:' prefix). - Update the ERR_DLOPEN_FAILED message for better_sqlite3.node to point at the shim. Fixes #14997 Fixes #16050
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughBun now resolves Changesbetter-sqlite3 compatibility
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- .raw()/.pluck()/.expand() iterate() now uses values() so duplicate column names (SELECT a.id, b.id FROM a JOIN b) keep their positional values instead of collapsing through an object key. - Expose sqlite3_stmt_readonly() as a native 'readonly' getter on the bun:sqlite statement and delegate Statement#readonly to it, so INSERT/UPDATE/DELETE correctly report readonly === false. - Use the shared throwNotImplemented helper for the stubbed methods. - Tighten the fileMustExist toThrow() assertion to expect SqliteError.
|
Updated 11:05 PM PT - Aug 1st, 2026
❌ @robobun, your commit 0ae892b has 2 failures in
🧪 To try this PR locally: bunx bun-pr 36712That installs a local version of the PR into your bun-36712 --bun |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/codegen/internal-module-registry-scanner.ts`:
- Line 18: Update the internal registry lookup in the scanner to test whether
the result is undefined rather than relying on truthiness, so module ID 0 from
internalRegistry.get("bun:ffi") is treated as a valid match. Preserve the
existing generated bun: key fallback and resolution behavior for actual misses.
In `@src/js/thirdparty/better-sqlite3.ts`:
- Line 216: Update the verbose handling in the better-sqlite3 shim so the
validated callback is invoked with the SQL source in both the prepare and exec
paths. Preserve the existing validation and ensure callers passing verbose
receive query logging rather than silently being ignored.
- Around line 76-87: Update get() to fetch only one row through the statement’s
single-row API, then apply the existing raw/pluck/expand mapping without
materializing values(). Update iterate() to consume this.#stmt.iterate() lazily
and map each yielded row. Retain values() only for the duplicate-column
positional case covered by the existing test, preserving lazy cursor behavior
otherwise.
- Around line 315-317: Update the unsafeMode method so calling unsafeMode(true)
throws an explicit unsupported-operation error, while unsafeMode(false) remains
a no-op that returns the current database instance.
In `@test/js/first_party/better-sqlite3/better-sqlite3.test.ts`:
- Around line 198-210: Add tests alongside the existing better-sqlite3 shim
coverage for expand() and expand().all() using the $ bucket, Database
constructed from a serialized buffer, statement safeIntegers(true), database
defaultSafeIntegers(true), serialize() round-tripping, and
Statement[Symbol.iterator] iteration. Exercise each entry point through its
expected behavior while preserving the current tests and keeping the cases near
their related variant coverage.
🪄 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: 354ef716-de6a-4e53-8af7-25272d62e2be
📒 Files selected for processing (8)
src/codegen/internal-module-registry-scanner.tssrc/install/default-trusted-dependencies.txtsrc/js/bun/sqlite.tssrc/js/thirdparty/better-sqlite3.tssrc/jsc/bindings/BunProcess.cppsrc/jsc/bindings/sqlite/JSSQLStatement.cppsrc/resolve_builtins/HardcodedModule.rstest/js/first_party/better-sqlite3/better-sqlite3.test.ts
💤 Files with no reviewable changes (1)
- src/install/default-trusted-dependencies.txt
- internal-module-registry scanner: treat module id 0 as a hit (bun/ffi.ts is alphabetically first and now receives id 0). - Wire the verbose callback so it is invoked with the SQL source on each run/get/all/iterate/exec. - unsafeMode(true) now throws ERR_NOT_IMPLEMENTED since bun:sqlite keeps SQLITE_DBCONFIG_DEFENSIVE on; unsafeMode(false) remains a no-op. - Add tests for expand(), Symbol.iterator, serialize()/Buffer round-trip, safeIntegers()/defaultSafeIntegers(), verbose, and unsafeMode.
- Named parameters (@name/:name/$name) bound via { name: value } now match
instead of silently binding NULL, and unknown keys throw.
- Remove the dead try/catch around PRAGMA busy_timeout (cannot fail on a
freshly-opened handle with a validated integer).
- bun:sqlite Database#transaction(fn, self): use self for the returned
function's .database back-reference when provided, so the better-sqlite3
shim's tx.database points at the shim instance instead of the internal
bun:sqlite handle.
- Remove the unreachable fallback branch in all() and fold all three mode methods into a single #mapRow(row) call. - Drop the unused #safeIntegers field and constructor parameter. - Wire verbose into pragma() too (test extended).
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js/thirdparty/better-sqlite3.ts:381-388— The third clauseSQLiteError[Symbol.hasInstance](instance)is redundant with the second: bun:sqlite'sSQLiteError[Symbol.hasInstance]is exactlyinstance?.name === "SQLiteError", and the outer guard already ensuresinstanceis a non-null object, so it reduces byte-for-byte toinstance.name === "SQLiteError". Drop one of the two — or, if the delegation is intentional future-proofing against bun:sqlite switching to a brand check, drop the string compare instead so there's exactly one source of truth.Extended reasoning...
What the issue is
SqliteError[Symbol.hasInstance]atsrc/js/thirdparty/better-sqlite3.ts:381-388accepts an instance when any of three conditions hold:instance != null && typeof instance === "object" && (instance.name === "SqliteError" || instance.name === "SQLiteError" || SQLiteError[Symbol.hasInstance](instance))
The third disjunct calls bun:sqlite's
SQLiteError[Symbol.hasInstance], whose entire implementation (src/js/bun/sqlite.ts) is:static [Symbol.hasInstance](instance) { return instance?.name === "SQLiteError"; }
The outer guard on lines 383-384 has already established
instance != null && typeof instance === "object", so the optional-chain ininstance?.nameis a no-op and the delegated call reduces to exactlyinstance.name === "SQLiteError"— byte-identical to the second clause. The third clause can therefore never make the disjunction true when the second is false; it is provably dead code introduced in this PR.Why nothing prevents it
SQLiteErroris imported at the top of the file frombun:sqliteand its[Symbol.hasInstance]is a static class method — there is no runtime configuration that changes what it does. For any non-hostile input (i.e.,.nameis not a getter that flips between reads), evaluating the second clause and the third clause produces the same boolean.Step-by-step proof
Take
errthrown bydb.exec("NOT VALID SQL")(a bun:sqliteSQLiteErrorwith.name === "SQLiteError"), and evaluateerr instanceof SqliteError:- Line 383:
err != null→ true. - Line 384:
typeof err === "object"→ true. - Clause 1:
err.name === "SqliteError"→ false (capital L). - Clause 2:
err.name === "SQLiteError"→ true. Short-circuit; result istrue.
Now take any
errwith.name === "Foo":- Guard passes.
- Clause 1: false. Clause 2: false.
- Clause 3:
SQLiteError[Symbol.hasInstance](err)→err?.name === "SQLiteError"→"Foo" === "SQLiteError"→ false.
For clause 3 to add anything, there would need to exist an
instancewhereinstance.name === "SQLiteError"is false butinstance?.name === "SQLiteError"is true — which is impossible for a non-null object whose.nameis a plain data property.Impact
None at runtime — one redundant property read + string compare per
instanceof SqliteErrorcheck. This is purely code hygiene in a new file this PR introduces. REVIEW.md's dead-code rule: "Every line you add must be demonstrably live."How to fix
The comment on line 379 ("Let
err instanceof SqliteErrormatch bun:sqlite'sSQLiteErrortoo") suggests the intent is delegation. Two consistent fixes:- Keep the delegation, drop the string compare — remove
instance.name === "SQLiteError" ||and keepSQLiteError[Symbol.hasInstance](instance). This future-proofs the shim if bun:sqlite ever switches itshasInstanceto a real brand check; the shim stays correct without a matching edit. - Keep the string compare, drop the delegation — remove
|| SQLiteError[Symbol.hasInstance](instance). Simpler and avoids the extra call, but hardcodes bun:sqlite's current implementation detail here.
Either way, one of the two should go so the disjunction contains no clause that is provably subsumed by another.
- Line 383:
… message - sqlite3_column_decltype() reads schema metadata and does not require a stepped row, so remove the hasExecuted guard from the native declaredTypes getter. The shim's columns() now reads it directly (type: 'INTEGER'/'TEXT'/... on a freshly prepared statement) instead of swallowing the throw and returning null. column-types.test.js updated. - Drop the redundant name === 'SQLiteError' clause from the SqliteError Symbol.hasInstance; the delegation to bun:sqlite's SQLiteError covers it. - Update scripts/handle-crash-patterns.ts to point better-sqlite3 crash reporters at the built-in module instead of saying it is unsupported.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js/thirdparty/better-sqlite3.ts:358-366— The third disjunctSQLiteError[Symbol.hasInstance](instance)is provably dead: bun:sqlite defines it asinstance?.name === "SQLiteError", and after the outerinstance != null && typeof instance === "object"guard that reduces to exactlyinstance.name === "SQLiteError"— the second disjunct immediately preceding it. Drop the third disjunct so the parenthesized check is just(instance.name === "SqliteError" || instance.name === "SQLiteError").Extended reasoning...
What the issue is
The shim's
SqliteError[Symbol.hasInstance]at lines 358-366 checks three disjuncts after establishinginstance != null && typeof instance === "object":(instance.name === "SqliteError" || instance.name === "SQLiteError" || SQLiteError[Symbol.hasInstance](instance))
The third disjunct calls into bun:sqlite's
SQLiteError[Symbol.hasInstance], which is defined insrc/js/bun/sqlite.tsas:static [Symbol.hasInstance](instance) { return instance?.name === "SQLiteError"; }
Why it can never be the deciding term
By the time the third disjunct is evaluated, two facts hold:
- The outer guard has established
instance != null && typeof instance === "object", so the optional-chain ininstance?.nameis moot — it's justinstance.name. ||short-circuits, so the second disjunctinstance.name === "SQLiteError"has already evaluated false.
Substituting: the third disjunct evaluates
instance.name === "SQLiteError", which fact (2) has already established is false. It can never be true when reached, so it is provably dead.There is no path around this.
SQLiteErroris captured at module load fromrequire("bun:sqlite")(line 2), and bun:sqlite'sSQLiteErrorclass is a fixed built-in whoseSymbol.hasInstanceis not user-overridable — no reentrant user code can change what the third disjunct computes.Step-by-step proof
Take a bun:sqlite-thrown error
ewithe.name === "SQLiteError":- Outer guard:
e != null✓,typeof e === "object"✓ → enter parenthesized OR. - First disjunct:
"SQLiteError" === "SqliteError"→ false. - Second disjunct:
"SQLiteError" === "SQLiteError"→ true, return true. Third disjunct never evaluated.
Take any object
owitho.name === "other":- Outer guard passes.
- First disjunct: false. Second disjunct: false.
- Third disjunct:
SQLiteError[Symbol.hasInstance](o)→o?.name === "SQLiteError"→"other" === "SQLiteError"→ false. Return false.
For every possible
instance.name, the third disjunct either isn't reached (name is"SQLiteError") or evaluates to false (name is anything else). Deleting it changes no observable behavior — theSqliteErrortest inbetter-sqlite3.test.ts(which checks bothSqliteError("oops", "SQLITE_TEST") instanceof SqliteErrorand a bun:sqlite-thrownSQLiteError instanceof SqliteError) continues to pass.Why flag it
REVIEW.md's dead-code rule is explicit: "Every line you add must be demonstrably live" and "Delete dead code in the same PR that makes it dead." This is a new file introduced by this PR, so the rule applies directly. The comment on line 357 ("Let
err instanceof SqliteErrormatch bun:sqlite'sSQLiteError") describes the intent the second disjunct already fully implements — the third disjunct looks like belt-and-suspenders that predates confirming what bun:sqlite's hasInstance actually does.Impact
None at runtime — pure code hygiene. One redundant cross-module property lookup + call on the (rare) miss path.
Fix
Drop the third disjunct:
(instance.name === "SqliteError" || instance.name === "SQLiteError")
- The outer guard has established
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/thirdparty/better-sqlite3.ts (1)
102-111: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate bound parameters when
iterate()is called.Because
iterate()is a generator,#params(args)runs only on the first.next().stmt.bind(1).iterate(2)must throw"This statement already has bound parameters"during theiterate()call. Move#params(args)outside the generator. Keep#trace()with row production because upstream invokes its logger on the first.next().🤖 Prompt for 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. In `@src/js/thirdparty/better-sqlite3.ts` around lines 102 - 111, Update the statement iterate flow around iterate and `#params` so bound-parameter validation occurs immediately when iterate() is called, before generator execution; preserve the existing "This statement already has bound parameters" error for calls such as stmt.bind(1).iterate(2). Keep `#trace`() inside the generator so logging still occurs on the first .next(), and continue using the validated parameters for row production.
🤖 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.
Outside diff comments:
In `@src/js/thirdparty/better-sqlite3.ts`:
- Around line 102-111: Update the statement iterate flow around iterate and
`#params` so bound-parameter validation occurs immediately when iterate() is
called, before generator execution; preserve the existing "This statement
already has bound parameters" error for calls such as stmt.bind(1).iterate(2).
Keep `#trace`() inside the generator so logging still occurs on the first .next(),
and continue using the validated parameters for row production.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 653ebfb9-5251-4291-894e-fa01bf7b0c57
📒 Files selected for processing (7)
scripts/handle-crash-patterns.tssrc/codegen/internal-module-registry-scanner.tssrc/js/bun/sqlite.tssrc/js/thirdparty/better-sqlite3.tssrc/jsc/bindings/sqlite/JSSQLStatement.cpptest/js/bun/sqlite/column-types.test.jstest/js/first_party/better-sqlite3/better-sqlite3.test.ts
💤 Files with no reviewable changes (1)
- src/jsc/bindings/sqlite/JSSQLStatement.cpp
- db.exec('')/db.exec('-- comment') now return this instead of throwing,
matching sqlite3_exec()'s documented no-op.
- pragma() applies defaultSafeIntegers to its internal statement.
- Update packages/bun-types/sqlite.d.ts and docs/runtime/sqlite.mdx to
reflect that declaredTypes no longer requires the statement to have
been executed.
There was a problem hiding this comment.
Beyond the inline findings, I also checked the internal-module-registry-scanner.ts change for wider impact: the startsWith("bun/") fix newly registers bun:ffi/bun:sql/bun:sqlite in the internal registry (previously the branch never matched), and none of those collide with _NativeModule.h entries; the directMatch !== undefined fix is needed because bun/ffi.ts now sorts to index 0. No other builtin currently require("bun:ffi"|"bun:sql"|"bun:sqlite"), so no existing module changes behavior.
Extended reasoning...
The three inline findings this run are all nit-severity compat divergences in the shim (O(N) materialization for .raw()/.pluck()/.expand() in iterate()/get(), missing REQUIRE_STATEMENT_RETURNS_DATA guard, and verbose passing raw source instead of sqlite3_expanded_sql). None block the drizzle path the PR targets. Separately I traced the codegen scanner edit — it's a shared build-time file, so I confirmed the newly-registered bun: specifiers don't overlap _NativeModule.h and no existing src/js file was already requiring them, so the change is scoped to enabling the new shim's require("bun:sqlite").
- get/all/iterate/pluck/raw/expand/columns now throw 'This statement does not return data. Use run() instead' when the prepared statement has zero result columns, matching better-sqlite3's REQUIRE_STATEMENT_RETURNS_DATA guard. A DELETE accessed via .all() is refused rather than executed. - The verbose callback now receives sqlite3_expanded_sql() (via Statement#toString()) after parameters are bound, so 'INSERT INTO t VALUES (?)'.run(1) logs 'INSERT INTO t VALUES (1)'.
…Names - run/get/all/iterate now wrap the native call in try/finally so verbose fires for failing statements too (matching sqlite3_trace_v2 firing at step start), and iterate() pumps the first .next() before tracing so toString() reflects the current bindings in object mode. - #expandRow caches columnNames once per Statement instead of re-reading the native getter per row.
- All verbose invocations now go through a single guarded trace() closure so a throwing logger cannot replace the primary result or mask an underlying SQLiteError. - .bind() copies TypedArray/Buffer arguments so mutating the caller's buffer before execution no longer changes what is inserted (SQLITE_TRANSIENT semantics).
Known divergences from better-sqlite3These are intentional gaps left for follow-up; none affect the drizzle-kit / drizzle-orm path this PR targets.
|
…ise on wider views)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/thirdparty/better-sqlite3.ts (1)
158-167: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
bind()snapshot missesUint8Array/Buffervalues nested in named-parameter objects.The loop only converts a top-level array element when
args[i] instanceof Uint8Array. When a caller binds named parameters through a single object argument, for examplestmt.bind({ $blob: buf }),args[0]is a plain object and the check does not descend into its properties. AUint8Array/Buffervalue inside that object is not snapshotted, so mutating the buffer after.bind()and before.run()/.get()produces stale or corrupted data for named blob parameters, even though positional blob parameters are correctly snapshotted.🛡️ Proposed fix to also snapshot named-parameter object values
for (let i = 0; i < args.length; i++) { - if (args[i] instanceof Uint8Array) args[i] = Buffer.from(args[i]); + if (args[i] instanceof Uint8Array) { + args[i] = Buffer.from(args[i]); + } else if (args[i] !== null && typeof args[i] === "object" && !Array.isArray(args[i])) { + const obj = args[i]; + for (const key in obj) { + if (obj[key] instanceof Uint8Array) obj[key] = Buffer.from(obj[key]); + } + } }Consider adding a test alongside
test/js/first_party/better-sqlite3/better-sqlite3.test.tsLines 78-85 that exercises.bind({ $blob: buf })with a mutated buffer, to lock in the same SQLITE_TRANSIENT-style guarantee for named parameters.🤖 Prompt for 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. In `@src/js/thirdparty/better-sqlite3.ts` around lines 158 - 167, Update bind() to snapshot Uint8Array/Buffer values inside named-parameter objects as well as top-level positional arguments, while preserving the existing single-bind guard and fluent return. Ensure object properties are converted before assigning this.#bound, and add coverage in the better-sqlite3 tests for mutating a buffer after bind({ $blob: buf }) but before execution.
🤖 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/thirdparty/better-sqlite3.ts`:
- Around line 299-311: Update Database#exec, specifically the this.exec
function, to avoid relying on the exact error message when handling
whitespace/comment-only SQL. Detect empty/comment-only input before calling
db.run(), or introduce and use a stable error code/property for that condition,
while preserving propagation of all other database errors.
---
Outside diff comments:
In `@src/js/thirdparty/better-sqlite3.ts`:
- Around line 158-167: Update bind() to snapshot Uint8Array/Buffer values inside
named-parameter objects as well as top-level positional arguments, while
preserving the existing single-bind guard and fluent return. Ensure object
properties are converted before assigning this.#bound, and add coverage in the
better-sqlite3 tests for mutating a buffer after bind({ $blob: buf }) but before
execution.
🪄 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: 01d44001-23ae-42af-8489-8973f57d5adc
📒 Files selected for processing (4)
docs/runtime/sqlite.mdxpackages/bun-types/sqlite.d.tssrc/js/thirdparty/better-sqlite3.tstest/js/first_party/better-sqlite3/better-sqlite3.test.ts
|
Build 87528 (0ae892b, the
Everything else is flaky (passed on retry/alone). Diff is green on the lanes it touches; ready for review. |
On Windows the database file handle stays open until every sqlite3_stmt is finalized, so the using-tempDir cleanup in the fileMustExist test hit EBUSY. Track the bun:sqlite statements created via prepare() and finalize them in close(), matching better-sqlite3's close() which iterates sqlite3_next_stmt.
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 `@src/js/thirdparty/better-sqlite3.ts`:
- Line 272: Replace the strong Set-based tracking in Database.prepare and
Database.close with lifecycle-aware tracking that allows unreachable raw
statements to be released while retaining and finalizing live statements on
close. Preserve native finalize behavior and add a regression test covering
repeated prepare-and-discard cycles to verify resources are not retained for the
database lifetime.
🪄 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: 8b418eb3-edab-4ccb-9e63-03dd6bff5cc0
📒 Files selected for processing (1)
src/js/thirdparty/better-sqlite3.ts
Long-running servers that never call close() would otherwise accumulate one WeakRef + Set slot per prepare() call unboundedly. Follows the same shape as src/js/node/diagnostics_channel.ts WeakRefMap.
There was a problem hiding this comment.
The FinalizationRegistry in 0ae892b resolves the dead-WeakRef accumulation I flagged last pass; no further findings from this run. Deferring to a maintainer for the product/API calls this PR makes — the documented known-divergences table, the own-property (vs prototype) method placement, and dropping better-sqlite3 from default-trusted-dependencies.txt.
Checked this pass:
- Statement lifecycle: WeakRef + FinalizationRegistry bounds the tracking Set to live statements;
close()derefs and finalizes survivors, and post-clear registry callbacks are harmless no-ops. declaredTypeshasExecuted-guard removal —sqlite3_column_decltypereads schema metadata and does not require a stepped row; docs/types updated to match.- Codegen scanner: the
"bun/"prefix fix anddirectMatch !== undefined(index 0 was previously falsy-rejected) both look correct. .name === ':memory:'for Buffer input was examined and ruled out as a bug.
Extended reasoning...
Overview
This PR adds a hardcoded better-sqlite3 module override (src/js/thirdparty/better-sqlite3.ts, ~435 lines) that wraps bun:sqlite with the better-sqlite3 Database/Statement API, so require('better-sqlite3') works without the V8-API native addon. Supporting changes: a native readonly getter on JSSQLStatement and removal of the hasExecuted guard on declaredTypes; module-resolver entries in HardcodedModule.rs; a codegen scanner fix so builtins can require('bun:sqlite'); removal of better-sqlite3 from default-trusted-dependencies.txt; updated dlopen error text; docs/types; and a 383-line test file plus updates to column-types.test.js.
Level of scrutiny
High — this is new user-facing API surface with intentional, documented divergences from the reference implementation (prototype-vs-own methods affecting APM instrumentation, prefixed named-parameter binding, .expand() grouping under $, changes type under safeIntegers, iterator early-return not resetting the native stmt, etc.). Those are all recorded in the PR's known-divergences comment and have been discussed across ~200 timeline events, but which ones are acceptable to ship is a maintainer call. Removing better-sqlite3 from the default-trusted list is also a product decision that changes bun install behavior for existing projects that opted into the real native build.
Security risks
Low. The shim delegates to bun:sqlite; the only filesystem interaction is existsSync(dirname(resolve(filename))) mirroring better-sqlite3's own directory check. loadExtension passes through to the existing bun:sqlite path (no new capability). nativeBinding is accepted and ignored. No new native allocation paths beyond the trivial sqlite3_stmt_readonly getter.
Other factors
The bug hunter found nothing this run. Over the PR's lifetime I raised a series of findings (raw/pluck iterate reading positional values, .readonly correctness, Buffer.from element-wise copy on wide TypedArrays, strong-ref statement leak, then dead-WeakRef accumulation) — each was fixed or logged as a known divergence, and 0ae892b closed the last one. Test coverage for the shim is thorough for the drizzle call shapes it targets and CI is green per the author's build-87520 note (the one Windows-aarch64 failure is unrelated). Given the scope and the design-tradeoff acceptances baked into the known-divergences table, this warrants a human maintainer's sign-off rather than auto-approval.
2 participants
What
better-sqlite3is a V8-API native addon, so its compiled.nodefile cannot be loaded by Bun (process.dlopenthrowsERR_DLOPEN_FAILED: "'better-sqlite3' is not yet supported in Bun", tracked in #4290). Its install script also falls back tonode-gyp rebuildwhen no prebuilt matches the reported Node ABI, which either spends minutes compiling a binary Bun cannot use, or fails to compile against the Node headers Bun reports:This adds a thirdparty override:
require("better-sqlite3")/import "better-sqlite3"now resolves to a shim that wrapsbun:sqlitewith the better-sqlite3Database/StatementAPI, including:new Database(filename | Buffer, { readonly, fileMustExist, timeout, verbose, nativeBinding })plus.name/.open/.inTransaction/.readonly/.memory.prepare/.exec/.close/.pragma/.transaction(with.deferred/.immediate/.exclusive) /.serialize/.loadExtension/.defaultSafeIntegers/.unsafeMode.run/.get/.all/.iterate, plus the chainable mode setters.raw()/.pluck()/.expand()/.bind()/.safeIntegers()and.columns()/.readerSqliteError(withinstanceofmatching forbun:sqlite'sSQLiteError).function/.aggregate/.table/.backupthrowERR_NOT_IMPLEMENTEDwith a pointer tobun:sqliteAlso:
better-sqlite3from the default-trusted-dependencies list sobun installno longer runs its install script by default. Users who want the real compile can still add it totrustedDependencies.require("bun:sqlite")(thesrc/js/bun/directory was never mapped to thebun:specifier prefix).ERR_DLOPEN_FAILEDmessage forbetter_sqlite3.nodeto point at the shim instead of justbun:sqlite.Verification
With the reporter's project (drizzle-kit 0.27.1 + drizzle-orm 0.36.0 + better-sqlite3 11.5.0), after this change:
Install goes from ~2m16s (failed) to ~4s, and
drizzle-kit migrateruns to completion.test/js/first_party/better-sqlite3/better-sqlite3.test.tscovers the shim API surface and the specific.prepare().bind().all()/.raw(bool).all()/.transaction()[behavior]()call shapes that drizzle-kit and drizzle-orm use.Fixes #14997
Fixes #16050
[review] gate passed · iteration 4 · 12 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 12 passed · 0 rejected · iteration 4
evidence per changed file