-
-
Notifications
You must be signed in to change notification settings - Fork 10
refactor: add SQLite adapter to decouple from bun:sqlite #970
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| # Bun → Node.js Migration Plan | ||
|
|
||
| Status: In progress. See below for completed and remaining steps. | ||
|
|
||
| ## Completed | ||
|
|
||
| ### Phase 4 (early): Package Manager Switch | ||
| - [x] Changed `packageManager` from `bun@1.3.13` to `pnpm@10.11.0` | ||
| - [x] Moved `patchedDependencies` into `pnpm` config section | ||
| - [x] Added `onlyBuiltDependencies: ["esbuild"]` | ||
| - [x] Added phantom deps as explicit devDependencies: `@sentry/core`, `@clack/prompts` | ||
| - [x] Generated `pnpm-lock.yaml` | ||
| - [x] Verified all patches apply (including cross-version: `@stricli/core@1.2.5` patch on `1.2.7`) | ||
|
|
||
| ### Phase 2, Group D: SQLite Adapter | ||
| - [x] Created `src/lib/db/sqlite.ts` — runtime-detecting adapter (bun:sqlite under Bun, node:sqlite under Node.js) | ||
| - [x] Updated 4 source files: `db/index.ts`, `schema.ts`, `migration.ts`, `utils.ts` | ||
| - [x] Updated 3 test files: `fix.test.ts`, `telemetry.test.ts`, `schema.test.ts` | ||
| - [x] Zero `bun:sqlite` imports remain in `src/` or `test/` | ||
|
|
||
| ## Remaining | ||
|
|
||
| ### Phase 2: Source Code Migration (replace Bun.* APIs in `src/`) | ||
|
|
||
| **Group A: File I/O** — Replace `Bun.file()` / `Bun.write()` with `node:fs/promises` | ||
| - `Bun.file(path).text()` → `readFile(path, "utf-8")` | ||
| - `Bun.file(path).json()` → `readFile(path, "utf-8")` then `JSON.parse()` | ||
| - `Bun.file(path).exists()` → `access(path).then(() => true, () => false)` | ||
| - `Bun.write(path, content)` → `writeFile(path, content)` | ||
| - Scan all of `src/` for occurrences | ||
|
|
||
| **Group B: Process/System APIs** — Replace Bun.which / Bun.spawn / Bun.sleep | ||
| - `Bun.which("cmd")` → `which` from a Node.js-compatible package or custom implementation | ||
| - `Bun.spawn()` / `Bun.spawnSync()` → `child_process.spawn()` / `spawnSync()` | ||
| - `Bun.sleep(ms)` → `setTimeout` promise wrapper | ||
|
|
||
| **Group C: Miscellaneous Bun APIs** | ||
| - `Bun.Glob` → `tinyglobby` or `picomatch` (already in devDependencies) | ||
| - `Bun.randomUUIDv7()` → `uuidv7` package (already in devDependencies) | ||
| - `Bun.semver.order()` → `semver.compare()` (already in devDependencies) | ||
| - `Bun.zstdCompressSync()` / `Bun.zstdDecompressSync()` → Node.js zlib or `zstd-napi` package | ||
|
|
||
| **Group E: Unpolyfilled APIs** | ||
| - `bspatch.ts` and `upgrade.ts` — Replace any Bun-specific APIs not covered by node-polyfills.ts | ||
|
|
||
| ### Phase 3: Test Migration (`bun:test` → Vitest) | ||
|
|
||
| - Add `vitest` as devDependency | ||
| - Replace `import { ... } from "bun:test"` with Vitest equivalents | ||
| - Replace `bun test` scripts with `vitest` | ||
| - Key differences: | ||
| - `bun:test`'s `mock.module()` → Vitest's `vi.mock()` | ||
| - `bun:test`'s `spyOn` → Vitest's `vi.spyOn()` | ||
| - Test file discovery patterns may differ | ||
| - `--isolate --parallel` behavior needs Vitest equivalent | ||
|
|
||
| ### Phase 4: CI & Dev Scripts (remaining) | ||
|
|
||
| - Update `package.json` scripts: `bun run` → `pnpm run` where appropriate | ||
| - Replace `bun run src/bin.ts` with `tsx src/bin.ts` (add `tsx` devDependency) | ||
| - Replace `bun run script/*.ts` with `tsx script/*.ts` | ||
| - Replace `bunx` with `pnpm exec` | ||
| - Update GitHub Actions workflows to use pnpm + Node.js instead of Bun | ||
| - Update `Dockerfile` / build scripts if applicable | ||
|
|
||
| ### Phase 5: Cleanup | ||
|
|
||
| - Remove `@types/bun` from devDependencies | ||
| - Remove `bun.lock` (replaced by `pnpm-lock.yaml`) | ||
| - Remove or update `script/node-polyfills.ts` (may become unnecessary) | ||
| - Update `AGENTS.md` Bun API reference table | ||
| - Remove Bun-specific `.cursor/rules/bun-cli.mdc` or update for Node.js | ||
| - Clean up any remaining `Bun.*` references in comments/docs | ||
|
|
||
| ## Known Issues | ||
|
|
||
| - `test/lib/index.test.ts` — `sdk.run throws when auth is required but missing` fails under pnpm's strict `node_modules`. The mock fetch returns empty 200s which prevents the expected auth error from being thrown. Pre-existing test fragility, not caused by migration changes. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| /** | ||
| * SQLite adapter wrapping node:sqlite's DatabaseSync with a convenient API. | ||
| * | ||
| * This module is the single import point for all SQLite access in the | ||
| * codebase. It provides a `.query(sql).get()` / `.all()` / `.run()` | ||
| * interface and a manual `transaction()` wrapper. | ||
| * | ||
| * Uses `node:sqlite` (Node 22+) as the backing implementation. Falls back | ||
| * to `bun:sqlite` when `node:sqlite` is unavailable (Bun runtime) — this | ||
| * fallback will be removed once the test runner migrates off Bun. | ||
| */ | ||
|
|
||
| /** Valid SQLite binding value */ | ||
| export type SQLQueryBindings = | ||
| | string | ||
| | number | ||
| | bigint | ||
| | null | ||
| | Uint8Array | ||
| | undefined; | ||
|
|
||
| /** | ||
| * Prepared statement wrapper exposing `.get()`, `.all()`, `.run()`. | ||
| */ | ||
| class StatementWrapper { | ||
| // biome-ignore lint/suspicious/noExplicitAny: backing driver types vary | ||
| private readonly stmt: any; | ||
|
|
||
| // biome-ignore lint/suspicious/noExplicitAny: backing driver types vary | ||
| constructor(stmt: any) { | ||
| this.stmt = stmt; | ||
| } | ||
|
|
||
| get( | ||
| ...params: SQLQueryBindings[] | ||
| ): Record<string, SQLQueryBindings> | undefined { | ||
| return this.stmt.get(...params) as | ||
| | Record<string, SQLQueryBindings> | ||
| | undefined; | ||
| } | ||
|
|
||
| all(...params: SQLQueryBindings[]): Record<string, SQLQueryBindings>[] { | ||
| return this.stmt.all(...params) as Record<string, SQLQueryBindings>[]; | ||
| } | ||
|
|
||
| run(...params: SQLQueryBindings[]): void { | ||
| this.stmt.run(...params); | ||
| } | ||
| } | ||
|
|
||
| /** Resolve the underlying SQLite constructor. */ | ||
| function getSqliteConstructor(): new (path: string) => { | ||
| exec(sql: string): void; | ||
| prepare(sql: string): unknown; | ||
| close(): void; | ||
| } { | ||
| try { | ||
| // Primary: node:sqlite (Node 22+) | ||
| return require("node:sqlite").DatabaseSync; | ||
| } catch { | ||
| // Fallback: bun:sqlite — remove once test runner migrates off Bun | ||
| return require("bun:sqlite").Database; | ||
| } | ||
| } | ||
|
|
||
| // biome-ignore lint/suspicious/noExplicitAny: resolved dynamically | ||
| const SqliteImpl: any = getSqliteConstructor(); | ||
|
|
||
| /** | ||
| * SQLite database wrapper. | ||
| * | ||
| * - `exec(sql)` — execute raw SQL (DDL, multi-statement) | ||
| * - `query(sql)` — prepare a statement → `.get()` / `.all()` / `.run()` | ||
| * - `close()` — close the connection | ||
| * - `transaction(fn)` — wrap a function in BEGIN/COMMIT/ROLLBACK | ||
| */ | ||
| export class Database { | ||
| // biome-ignore lint/suspicious/noExplicitAny: backing driver resolved at runtime | ||
| private readonly db: any; | ||
|
|
||
| constructor(path: string) { | ||
| this.db = new SqliteImpl(path); | ||
| } | ||
|
|
||
| exec(sql: string): void { | ||
| this.db.exec(sql); | ||
|
Check failure on line 86 in src/lib/db/sqlite.ts
|
||
| } | ||
|
|
||
| query(sql: string): StatementWrapper { | ||
| // node:sqlite uses .prepare(), bun:sqlite uses .query() | ||
| const prepFn = this.db.prepare ?? this.db.query; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nullish coalescing operands reversed in query methodMedium Severity The expression Reviewed by Cursor Bugbot for commit 972b864. Configure here. |
||
| return new StatementWrapper(prepFn.call(this.db, sql)); | ||
| } | ||
|
|
||
| close(): void { | ||
| this.db.close(); | ||
| } | ||
|
|
||
| transaction<T>(fn: () => T): () => T { | ||
| // bun:sqlite has native transaction(); node:sqlite does not | ||
| if (typeof this.db.transaction === "function") { | ||
| return this.db.transaction(fn); | ||
| } | ||
| return () => { | ||
| this.db.exec("BEGIN"); | ||
| try { | ||
| const result = fn(); | ||
| this.db.exec("COMMIT"); | ||
| return result; | ||
| } catch (error) { | ||
| this.db.exec("ROLLBACK"); | ||
| throw error; | ||
| } | ||
|
Check warning on line 113 in src/lib/db/sqlite.ts
|
||
|
Comment on lines
+107
to
+113
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ROLLBACK exec failure silently discards the original transaction error If VerificationLines 107-113 of the manual node:sqlite transaction path: the Identified by Warden find-bugs · HZ6-E88 |
||
| }; | ||
| } | ||
| } | ||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isSchemaErrorandisReadonlyErrornever trigger under Node.js, silently disabling auto-repairThe error-classification functions in
schema.ts(lines 595, 615) gate onerror.name === 'SQLiteError', which is thebun:sqliteerror class name.node:sqlitethrows plainErrorinstances witherror.code === 'ERR_SQLITE_ERROR'anderror.name === 'Error', so both functions always returnfalseon Node.js — disabling the entire auto-repair system and read-only detection. Add a check forerror.code === 'ERR_SQLITE_ERROR'alongside the existing name check inschema.ts.Verification
Traced the full path:
Database.exec()(sqlite.ts:84) andStatementWrapper.run()(sqlite.ts:49) are now the only error-throwing surfaces for callers. Under node:sqlite, all SQLite-level errors are thrown as standardErrorinstances witherr.code === 'ERR_SQLITE_ERROR'(per Node.js 22 docs and source). Under bun:sqlite they areSQLiteErrorinstances.schema.ts:595checkserror instanceof Error && error.name === 'SQLiteError'— this is the bun:sqlite class name, and will never be true for node:sqlite errors. ConsequentlytryRepairAndRetry(),isSchemaError(), andisReadonlyError()all silently returnfalseon Node.js, meaning: (a) schema auto-repair never runs, (b) read-only database errors are never surfaced correctly. No test exists that exercises these functions with a real node:sqlite error object.Identified by Warden find-bugs · 9N4-5DY