fix(sqlite): review fixes for #710 — restore the bun:sqlite driver path - #839
Closed
sroussey wants to merge 2 commits into
Closed
fix(sqlite): review fixes for #710 — restore the bun:sqlite driver path#839sroussey wants to merge 2 commits into
sroussey wants to merge 2 commits into
Conversation
…condition
The migration to `node:sqlite` deleted `src/storage/bun.ts`, its
`_sqlite/bun.ts` adapter and the `"bun"` export condition on `./storage`, on
the premise that both runtimes could share one driver. Bun cannot: no tagged
release carries the `node:sqlite` builtin — `import("node:sqlite")` fails with
"No such built-in module" on 1.3.11 through `latest`, and only the rolling
canary has it. With the condition gone a Bun consumer resolved
`dist/storage/node.js` and got a driver whose dynamic import throws, so every
SQLite-backed path failed at `Sqlite.init()`.
Measured on the storage section under Bun 1.3.11 (`bun scripts/test.ts storage
bun`): 1716 pass / 20 fail / 7 errors before, 1960 pass / 0 fail after, against
2072 pass / 0 fail on the pre-migration main.
The restored adapter is the pre-migration one, so the two runtimes diverge
again exactly where they did before — better-sqlite3 error-code spellings,
option validation, the async-body refusal, BigInt narrowing — all of which the
Node driver bridges for itself and `bun:sqlite` either provides natively or
does not need.
Consequently `engines.bun` comes back down to `^1.3.11` and `packageManager` to
`bun@1.3.11`: the canary floor only ever existed to reach `node:sqlite`, and
`bun@1.4.0-canary.1` names a version no registry publishes, so `bun i` had no
installable toolchain. The two `bun-version: canary` CI pins raised for the
same reason go back to `latest`, restoring both workflows byte-for-byte to
main. The Node floor stays `>=24`, where `node:sqlite` is stable.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HJRf3YFa8DjmjsZvXz8xDT
…contract `BunExportConditions.test.ts` is the exact-set fixture for every `"bun"` export condition in the monorepo, so restoring `@workglow/sqlite ./storage` fails it until the fixture says three entries again. It also re-asserts which bundle the condition names — `dist/storage/bun.js` for Bun, `dist/storage/node.js` for Node — because the exact-set check alone would pass on a condition pointing at the wrong build. The prose the fixture's comment names (`.claude/CLAUDE.md`, both spots in `docs/technical/19-build-system.md`, and `docs/technical/18-multi-runtime-abstraction.md`) moves with it. `SqliteDriver.contract.test.ts` runs under both runners, so with two drivers back it now asserts one shared contract plus a node-scoped remainder. The split is drawn from measurement, not from the module list: forcing the node groups on under Bun fails 11 of 16 assertions, and the other 5 — savepoint nesting from an `exec`'d or prepared `BEGIN`, recovery after a stray `COMMIT`, inner-only unwind, and `all()` row prototypes — pass, so they moved into the shared group rather than staying skipped. That matters most for savepoint nesting, which `SqliteTabularStorage` depends on and which would otherwise have had no Bun coverage at all. What stays node-only is what genuinely diverges: better-sqlite3 error-code spellings, constructor-option rejection, the async-body refusal, the BigInt narrowing `node:sqlite` forces, and the busy-timeout contention window. Bun 1.3.11: 11 pass / 11 skip / 0 fail. Node (vitest): 22 pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HJRf3YFa8DjmjsZvXz8xDT
Collaborator
Author
|
bun 1.4 has node:sqlite |
Coverage Report
File CoverageNo changed files found. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #710 (
claude/better-sqlite3-node-sqlite-0ydg8g) so the diff here is just the review fixes. Merge into that branch.What this fixes
1. The migration removed Bun's only working SQLite driver
#710 deleted
providers/sqlite/src/storage/bun.ts, its_sqlite/bun.tsadapter and the"bun"export condition on./storage, on the premise that both runtimes could share onenode:sqlitedriver. Bun cannot. No tagged Bun release carries thenode:sqlitebuiltin — only the rolling canary does:With the condition gone, a Bun consumer resolved
dist/storage/node.jsand got a driver whose dynamic import throws, so every SQLite-backed path failed atSqlite.init(). Measured on the storage section under Bun 1.3.11, all three trees built from source:bun scripts/test.ts storage bunorigin/main(pre-migration, hasbun:sqlite)The restored adapter is the pre-migration one byte-for-byte, plus a comment explaining why it outlived the migration. Resolution now splits per runtime again —
BunSqliteDatabaseunder Bun,NodeSqliteDatabaseunder Node — verified directly by importing@workglow/sqlite/storagefrom each.2. The canary toolchain pin was unusable, and only existed for
node:sqliteengines.buncomes back down to^1.3.11andpackageManagertobun@1.3.11.bun@1.4.0-canary.1names a version no registry publishes — #710's own commit message says so — sobun ihad no installable toolchain to name. The Node floor stays>=24, wherenode:sqliteis stable and where the Node driver still needs it.The two
bun-version: canaryCI pins raised for the same reason go back tolatest, restoringnightly-typecheck.ymlandpublish-preview.yml-offbyte-for-byte tomain(blob hashes match).test.ymlandnightly-bun-parity.ymlare not touched — #710's merge withmainalready left them onlatest, so there was nothing to revert.3. Guards, drawn from measurement
BunExportConditions.test.tsis the exact-set fixture for every"bun"condition in the monorepo, so restoring@workglow/sqlite ./storagefails it until the fixture says three entries again. It also re-asserts which bundle the condition names (dist/storage/bun.jsvsdist/storage/node.js), because the exact-set check alone would pass on a condition pointing at the wrong build. The prose the fixture's own comment names moves with it:.claude/CLAUDE.md, both spots indocs/technical/19-build-system.md, anddocs/technical/18-multi-runtime-abstraction.md(plus thepackages/storageREADME, which described Bun as loadingnode:sqlite).SqliteDriver.contract.test.tsruns under both runners, so with two drivers back it now asserts one shared contract plus a node-scoped remainder. The split is drawn from measurement rather than from the module list: forcing the node groups on under Bun fails 11 of 16 assertions, and the other 5 pass — savepoint nesting from anexec'd or preparedBEGIN, recovery after a strayCOMMIT, inner-only unwind, andall()row prototypes. Those 5 moved into the shared group rather than staying skipped. That matters most for savepoint nesting, whichSqliteTabularStoragedepends on and which would otherwise have had no Bun coverage at all.What stays node-only is what genuinely diverges: better-sqlite3 error-code spellings, constructor-option rejection, the async-body refusal, the BigInt narrowing
node:sqliteforces, and the busy-timeout contention window.Verification
Run from the worktree root against the committed tree, dist mode (
bun run build), real results.bun run build:typesbun run buildbun scripts/test.ts storage vitestbun scripts/test.ts util vitestbun scripts/test.ts storage bunbun teston the two guard files (Bun 1.3.11)vitest runonSqliteDriver.contract.test.ts(Node)bun scripts/test.ts --check-sectionsbunx eslinton all 4 touched source filesbunx prettier --checkon all 12 touched filesTwo notes on results that look like failures and are not:
bun scripts/test.ts util bunreports 8 failures. They are pre-existing and unrelated (vi.advanceTimersByTimeAsyncis undefined under Bun's runner;dataUriToBlob/bytesToBase64Buffer-path cases). The pristine Migrate SQLite driver from better-sqlite3 to node:sqlite #710 head produces the identical 701 pass / 10 skip / 8 fail with the same 8 names.bun scripts/test.ts storage bunprints "exit code 99" with 0 failures. Pre-existing:origin/mainprints the same line, also with 0 failures.Prettier flags
.claude/CLAUDE.mdanddocs/technical/19-build-system.md, but both were already unformatted on the pristine base, in regions this PR does not touch (a table at line 32, JSON blocks, emphasis markers at line 275). Left alone rather than reformatted as unrelated noise.🤖 Generated with Claude Code
https://claude.ai/code/session_01HJRf3YFa8DjmjsZvXz8xDT
Generated by Claude Code