fix(core): replace better-sqlite3 with node:sqlite in the sqlite adapter#2106
fix(core): replace better-sqlite3 with node:sqlite in the sqlite adapter#2106gruntlord5 wants to merge 2 commits into
Conversation
better-sqlite3's natively compiled binary breaks deployments the moment the environment and the binary disagree: NODE_MODULE_VERSION mismatches after Node upgrades (emdash-cms#562, emdash-cms#582) and glibc version errors on shared hosting where the OS is older than the build machine (emdash-cms#1833). Using the Node.js built-in driver removes the native dependency entirely. The sqlite() adapter and the CLI's createDatabase() now open databases through node:sqlite's DatabaseSync, wrapped in a small better-sqlite3- compatible shim (node-sqlite-compat.ts) so Kysely's built-in SqliteDialect is unchanged. Statement .reader is derived from StatementSync.columns(), preserving INSERT ... RETURNING semantics. - better-sqlite3 moves to a devDependency (tests still use it directly) - drops the NODE_MODULE_VERSION rebuild hint (no native binary anymore) - removes better-sqlite3/bindings/file-uri-to-path from bundler externals - emdash package now declares engines node >=22.15 (unflagged node:sqlite + StatementSync.columns()) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QSivQavm4D4nK86DgJBJCX
🦋 Changeset detectedLatest commit: d8f835b The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
review/needs-review
Scope checkThis PR changes 686 lines across 13 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the right change for the problem described in #402. Moving the SQLite path from better-sqlite3 to Node's built-in node:sqlite removes a native compiled dependency that causes real deployment pain (NODE_MODULE_VERSION mismatches, glibc issues on shared hosting), and the small better-sqlite3-compat wrapper is a pragmatic way to keep Kysely's SqliteDialect unchanged.
I checked the wrapper, the CLI/runtime adapter wiring, the package changes, the Vite/tsdown externalization, and the tests. The approach is sound and the package-level changes (engines: node>=22.15, moving better-sqlite3 to devDependencies, updating NODE_NATIVE_EXTERNALS, the changeset) are all correct.
However, there are two issues that should be addressed before merge:
-
The test harness still runs on
better-sqlite3.packages/core/tests/utils/test-db.tscreates its SQLite test DB withimport Database from "better-sqlite3", so the existing database suite is not exercising the new node:sqlite/wrapper path. This contradicts the PR description's claim that "the existing 85 database tests now also run through it" and leaves the production driver under-tested. The harness should switch toopenNodeSqliteDatabase(":memory:"). -
The parameter wrapper is too permissive for booleans.
toBindingscasts Kysely's values withas SQLInputValue[], but theSQLInputValuetype excludesboolean.better-sqlite3accepts JS booleans (stored as0/1);node:sqliterejects them at bind time, so any query with a boolean value will throw at runtime. The wrapper should map booleans explicitly.
A few other cleanup notes (non-blocking):
pnpm-lock.yamlcontains a lot of unrelated churn (removed top-leveloverrides/patchedDependencies, manyhasBin/libcsnapshot changes). If these are from an unintentionalpnpm installregeneration, revert them to keep the change focused; if they're intentional, call them out in the PR.- The runtime adapter in
packages/core/src/db/sqlite.tsstill doesn't enable WAL/foreign keys, while the CLI path inconnection.tsdoes. This isn't a regression from the PR, but it's worth centralizing now that DB creation lives in one place. - Several templates/demos/e2e fixtures still list
better-sqlite3as a dependency. Since they no longer need it at runtime, consider removing it so new sites don't pull the native binary.
|
|
||
| import { openNodeSqliteDatabase } from "../../../src/db/node-sqlite-compat.js"; | ||
|
|
||
| describe("openNodeSqliteDatabase", () => { |
There was a problem hiding this comment.
[needs fixing] The new unit tests cover the wrapper in isolation, but the shared test harness in packages/core/tests/utils/test-db.ts still uses import Database from "better-sqlite3" for its SQLite databases. That means the existing database test suite is not actually running through the node:sqlite driver, which contradicts the PR description and leaves the production code path under-tested.
Switch the harness to create SQLite test databases through the wrapper so the full suite exercises the driver that ships with the package:
| describe("openNodeSqliteDatabase", () => { | |
| // In packages/core/tests/utils/test-db.ts | |
| import { openNodeSqliteDatabase } from "../../src/db/node-sqlite-compat.js"; | |
| export function createTestDatabase(): Kysely<DatabaseSchema> { | |
| resetSchemaCachesForTests(); | |
| const sqlite = openNodeSqliteDatabase(":memory:"); | |
| return new Kysely<DatabaseSchema>({ | |
| dialect: new SqliteDialect({ | |
| database: sqlite, | |
| }), | |
| }); | |
| } |
| }, | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
[needs fixing] toBindings casts Kysely's parameters to SQLInputValue[] without any value mapping, but SQLInputValue excludes boolean. better-sqlite3 accepts JavaScript booleans and stores them as 0/1; node:sqlite rejects booleans at bind time, so any query that inserts, updates, or selects on a boolean value will throw a runtime TypeError. This is a real compatibility gap for any code that relied on the old driver's coercion.
Map booleans to integers before spreading them into the statement:
| function toBindings(parameters: ReadonlyArray<unknown>): SQLInputValue[] { | |
| return parameters.map((value) => { | |
| if (typeof value === "boolean") return value ? 1 : 0; | |
| return value as SQLInputValue; | |
| }); | |
| } |
github-actions
Bot
added
review/awaiting-author
Test harness: packages/core/tests/utils/test-db.ts created its SQLite
databases with better-sqlite3, so the suite never exercised the driver
the package actually ships. It now goes through openNodeSqliteDatabase.
The three tests that use better-sqlite3 directly keep it as a devDep.
Parameter binding: node:sqlite binds a narrower set of JS types than
better-sqlite3, and differs in both directions, so toBindings now maps
instead of casting.
- undefined -> NULL. better-sqlite3 accepted it, node:sqlite throws.
Reachable from ordinary Kysely usage (`.where(col, "=", undefined)`
from an unset optional filter), so this was a real regression.
- Date -> actionable TypeError. better-sqlite3 threw; node:sqlite
silently binds NULL, turning a loud programming error into silent
data loss.
- boolean -> 0/1. Note the review's premise was inverted:
better-sqlite3 12.8.0 also rejects booleans ("SQLite3 can only bind
numbers, strings, bigints, buffers, and null"), so this is not a
regression from this PR. It is a pre-existing bug — the plugin
storage query API advertises `boolean` in WhereValue
(plugins/types.ts) but threw at bind time on either driver — and
mapping here fixes it.
Pragmas: journal_mode=WAL, busy_timeout=5000 and foreign_keys=ON move
into openNodeSqliteDatabase, the single place the package opens SQLite.
Previously only the CLI path set them, so sites on the runtime sqlite()
adapter silently got none of them.
Lockfile: regenerated with the repo's pinned pnpm (11.9.0 via corepack)
rather than the pnpm 9 that produced the original diff. pnpm 10+ reads
overrides/patchedDependencies from pnpm-workspace.yaml, which pnpm 9
ignores, which is why they were dropped from the lockfile — taking the
"@ungap/structured-clone" CWE-502 security pin and the @sigstore/core
patch with them. The lockfile diff against main is now only the
better-sqlite3 importer moves.
Also drops better-sqlite3 from the 9 templates/demos/fixtures that
listed it as a runtime dependency but never imported it, so new sites
no longer pull the native binary.
Verified: 4760 tests pass (the one failure, virtual-modules.test.ts,
fails identically on the unmodified PR tip — a macOS /var symlink
issue). Query-count harness matches its snapshots exactly, and WAL is
confirmed applied on the built fixture's data.db.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JSxrM3SUUGvrr9dzTQrbmF
github-actions
Bot
added
review/needs-rereview
2 participants
What does this PR do?
Replaces better-sqlite3 with Node's built-in
node:sqlitedriver in thesqlite()adapter and the CLI'screateDatabase(). A small better-sqlite3-compatible wrapper (node-sqlite-compat.ts, ~70 lines) lets Kysely's built-inSqliteDialectrun unchanged — statement.readeris derived fromStatementSync.columns(), soINSERT ... RETURNINGkeeps working.This removes the native compiled dependency that breaks deployments when the environment and binary disagree:
NODE_MODULE_VERSIONrebuild errors after Node upgrades (#562, #582) and the glibc hard-blocker on shared hosting (#1833). Discussion: #402.Notes:
NODE_MODULE_VERSIONrebuild-hint helper is removed (no native binary to rebuild).emdashnow declaresengines: node >=22.15(unflaggednode:sqlite+StatementSync.columns()).Type of change
Checklist
pnpm typecheckpasses (no errors in changed files; remaining errors identical onmain)pnpm lintpassespnpm testpasses (4751 passed; 1 pre-existing failure identical on cleanmain)pnpm formathas been runRETURNINGreader semantics; the existing 85 database tests now also run through it)AI-generated code disclosure
Screenshots / test output
Full core suite: 4751 passed / 1 failed — the failure is a pre-existing macOS temp-path test issue, verified identical on clean
main. End-to-end validation onfixtures/perf-site(sqlite+node target):emdash init+emdash seed+astro buildall succeed, and the built server serves seeded pages correctly.