refactor: abstract SQLite behind runtime-conditional #db import#18316
refactor: abstract SQLite behind runtime-conditional #db import#18316
Conversation
260be87 to
c2ce3b1
Compare
Greptile SummaryThis PR introduces a runtime-conditional
Confidence Score: 2/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[db.ts - Database.Client lazy init] --> B{Runtime}
B -->|bun| C[db.bun.ts\ninit - bun:sqlite + drizzle bun-sqlite]
B -->|node| D[db.node.ts\ninit - node:sqlite + drizzle node-sqlite]
B -->|default| C
A --> E[migrate - drizzle-orm/bun-sqlite/migrator]
E -->|⚠️ NOT abstracted| F{passes Bun migrator\nto Node.js client on Node}
C --> G[Returns SQLiteBunDatabase]
D --> H[Returns node-sqlite DB]
G --> I[db.run PRAGMA calls]
H --> I
I --> J[migrate db entries]
E --> J
style E fill:#ff6b6b,color:#fff
style F fill:#ff6b6b,color:#fff
Last reviewed commit: "refactor: abstract S..." |
| import { Database as BunDatabase } from "bun:sqlite" | ||
| import { drizzle, type SQLiteBunDatabase } from "drizzle-orm/bun-sqlite" | ||
| import { type SQLiteBunDatabase } from "drizzle-orm/bun-sqlite" | ||
| import { migrate } from "drizzle-orm/bun-sqlite/migrator" |
There was a problem hiding this comment.
Bun-specific migrator not abstracted behind
#db
The whole point of this PR is to abstract the runtime-specific database layer, but migrate is still imported directly from drizzle-orm/bun-sqlite/migrator. On line 104, migrate(db, entries) is called passing whatever init() returned — which, on Node.js, will be a drizzle-orm/node-sqlite client — into a Bun-specific migrator. The two adapters are typed differently and this is likely to fail at runtime under Node.js.
The migrator should either be exported from #db alongside init, or a separate #migrator conditional import should be added to package.json.
| import { migrate } from "drizzle-orm/bun-sqlite/migrator" | |
| import { migrate } from "#db/migrator" |
And correspondingly, db.bun.ts should re-export from drizzle-orm/bun-sqlite/migrator and db.node.ts from drizzle-orm/node-sqlite/migrator.
| @@ -1,5 +1,4 @@ | |||
| import { Database as BunDatabase } from "bun:sqlite" | |||
| import { drizzle, type SQLiteBunDatabase } from "drizzle-orm/bun-sqlite" | |||
| import { type SQLiteBunDatabase } from "drizzle-orm/bun-sqlite" | |||
There was a problem hiding this comment.
Bun-specific type alias persists for
Client
SQLiteBunDatabase is imported and used as the Client type alias (line 40: type Client = SQLiteBunDatabase). Under Node.js, init() from db.node.ts returns a BetterSQLite3Database<Record<string, never>> (or equivalent drizzle node-sqlite type), not a SQLiteBunDatabase. This mismatch means all Client-typed values, including the $client access in close(), are typed against the wrong runtime object.
The fix would be to either export a common DB type from #db or use ReturnType<typeof init> to infer the correct type.
| export function close() { | ||
| const sqlite = state.sqlite | ||
| if (!sqlite) return | ||
| sqlite.close() | ||
| state.sqlite = undefined | ||
| Client().$client.close() | ||
| Client.reset() | ||
| } |
There was a problem hiding this comment.
close() is no longer a safe no-op when DB is uninitialized
The original implementation guarded against uninitialized state with if (!sqlite) return. The new implementation calls Client(), which lazily initializes the database if it hasn't been opened yet. This means:
- If
close()is called before the database has ever been accessed, it will now open the database, then immediately close it — a behavioral regression. - If
close()is called a second time (afterClient.reset()), it will re-open the database again before closing it, rather than being a no-op.
A simple guard can restore the original safety:
| export function close() { | |
| const sqlite = state.sqlite | |
| if (!sqlite) return | |
| sqlite.close() | |
| state.sqlite = undefined | |
| Client().$client.close() | |
| Client.reset() | |
| } | |
| export function close() { | |
| if (!Client.loaded) return | |
| Client().$client.close() | |
| Client.reset() | |
| } |
(Requires exposing a loaded flag on the lazy wrapper, or checking via a separate flag.)
Replace direct bun:sqlite usage with a #db package import that resolves to either db.bun.ts (bun:sqlite) or db.node.ts (node:sqlite) depending on the runtime, enabling the database layer to work under both Bun and Node.js.
c2ce3b1 to
cca7440
Compare
Replace direct bun:sqlite usage with a #db package import that resolves to either db.bun.ts (bun:sqlite) or db.node.ts (node:sqlite) depending on the runtime, enabling the database layer to work under both Bun and Node.js.