Skip to content

fix(desktop): dual-ABI prebuilt for better-sqlite3 (Node + Electron)#96

Merged
hqhq1025 merged 2 commits intomainfrom
fix/sqlite-abi-dual-target
Apr 19, 2026
Merged

fix(desktop): dual-ABI prebuilt for better-sqlite3 (Node + Electron)#96
hqhq1025 merged 2 commits intomainfrom
fix/sqlite-abi-dual-target

Conversation

@hqhq1025
Copy link
Copy Markdown
Collaborator

Problem

better-sqlite3 ships exactly one .node binary which can target either Node's ABI or Electron's ABI but not both. We have two consumers of it:

  • Electron 33 main process — needs ABI 130 to run pnpm dev without the Could not open the local snapshots database… NODE_MODULE_VERSION 127, version requires 130 boot dialog.
  • Vitest (Node 22.11) — needs ABI 127 to run the 108 snapshot-related tests.

Whichever runtime got rebuilt last broke the other one. Pre-commit hooks have been failing all day because of this and PRs have been merged with --no-verify to work around it.

Fix (Option 1: dual prebuilt + per-runtime nativeBinding)

  • New apps/desktop/scripts/install-sqlite-bindings.cjs runs as a postinstall and stages two prebuilds side by side under better-sqlite3/build/Release/:
    • better_sqlite3.node-node.node — Node ABI, used by vitest
    • better_sqlite3.node-electron.node — Electron ABI, used by the desktop app
      Downloads via prebuild-install (already a transitive dep of better-sqlite3, so no new package added). Idempotent — a schemaVersion'd lock file lets the script no-op on warm caches.
  • snapshots-db.ts dispatches on process.versions.electron and passes the correct path through better-sqlite3's nativeBinding constructor option.

Picked over Option 2 (mock in tests) because it keeps tests exercising real SQLite and over Option 3 (node:sqlite swap) because Electron 33 still ships Node 20.18 and doesn't expose node:sqlite.

Verification

  • pnpm test288/288 pass (incl. 26 + 81 + 1 = 108 snapshot tests). Verified to still pass after manually overwriting the default better_sqlite3.node with the Electron-ABI binary, proving the dispatch is doing real work.
  • Direct sanity check from a Node process: passing the Electron binding path is rejected with the expected NODE_MODULE_VERSION mismatch; passing the Node path loads and CREATE TABLE works.
  • pnpm typecheck ✅ · pnpm lint ✅ · pre-commit hook (pnpm test) ✅ ran on this commit without --no-verify.

PRINCIPLES checks

  • Compatibility ✅ — uses better-sqlite3 v11's documented nativeBinding option; works on macOS arm64/x64, Windows x64/arm64, Linux x64 (all platforms prebuild-install supports). Lock file is schemaVersion-tagged for migration.
  • Upgradeability ✅ — bumping Electron or Node just changes the recorded versions; postinstall re-fetches matching prebuilds. No source-build toolchain required.
  • No bloat ✅ — zero new npm dependencies (reuses existing prebuild-install). The two extra .node files live only in node_modules and are not packaged into the installer.
  • Elegance ✅ — single 6-line dispatch in snapshots-db.ts; postinstall is one self-contained script with an idempotency lock.

better-sqlite3 ships a single .node binary that can target either Node's
ABI or Electron's ABI but not both. Electron 33 needs ABI 130 for `pnpm
dev` and Vitest (Node 22.11) needs ABI 127 for `pnpm test`, so any
electron-rebuild flips the binary and breaks the other consumer. Result:
pre-commit hooks have been failing all day with "NODE_MODULE_VERSION 130
... requires 127" and folks have been bypassing them with --no-verify.

Fix: stage two prebuilds side by side at install time, then opt into
the right one at open() via better-sqlite3's `nativeBinding` option.

- scripts/install-sqlite-bindings.cjs (postinstall, idempotent) downloads
  Node + Electron prebuilds via prebuild-install (already a transitive
  dep, so no new package added) and stashes them as
  better_sqlite3.node-{node,electron}.node next to the default binary.
  A schemaVersion'd lock file lets the script skip on warm caches.
- snapshots-db.ts dispatches on `process.versions.electron` to pass the
  matching binding path. The default binary stays in place as a fallback
  for any consumer that doesn't go through this module.

Verified:
- `pnpm test` → 288/288 pass (incl. 108 snapshots tests) even after
  swapping the default binary to the Electron-ABI one.
- Node rejects the Electron binary with the expected
  NODE_MODULE_VERSION mismatch when pointed at it directly, confirming
  the dispatch is doing real work.
- `pnpm typecheck` and `pnpm lint` clean.
Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [Major] postinstall hard-fails when electron is not installed (e.g. production-only installs), blocking install workflows — apps/desktop/scripts/install-sqlite-bindings.cjs:43
    Suggested fix:

    function resolveElectronVersion() {
      try {
        return require('electron/package.json').version;
      } catch {
        return null;
      }
    }
    
    // ...
    const electronVersion = resolveElectronVersion();
    if (electronVersion) {
      log(`downloading Electron prebuild (electron=${electronVersion}, ${platform}-${arch})`);
      downloadPrebuild({
        pkgDir,
        runtime: 'electron',
        target: electronVersion,
        arch,
        platform,
        dest: electronBinary,
      });
    } else {
      log('electron not installed; skipping Electron ABI prebuild');
    }
  • [Minor] Missing runtime-context error for native binding resolution makes ABI failures harder to diagnose and violates the “throw with context” constraint — apps/desktop/src/main/snapshots-db.ts:46
    Suggested fix:

    function openDatabase(filename: string, options?: BetterSqlite3.Options): Database {
      const Database = require('better-sqlite3') as typeof BetterSqlite3;
      const nativeBinding = resolveNativeBinding();
      try {
        return new Database(filename, { ...options, nativeBinding });
      } catch (cause) {
        throw new Error(
          `Failed to open SQLite database with native binding (${nativeBinding})`,
          { cause: cause instanceof Error ? cause : new Error(String(cause)) },
        );
      }
    }

Summary

  • Review mode: initial
  • Two issues were found in the latest diff: one install-time regression risk in the new postinstall flow and one missing error-context guard in native binding open path.
  • docs/VISION.md and docs/PRINCIPLES.md: Not found in repo/docs for this checkout.

Testing

  • Not run (automation)
  • Suggested: add Vitest coverage for resolveNativeBinding selection (Node vs Electron runtime) and for installer behavior when electron is absent.

open-codesign Bot

return path.dirname(pkgJsonPath);
}

function resolveElectronVersion() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolveElectronVersion() currently throws when electron is not installed (common in production-only installs), causing postinstall to fail hard. Consider making Electron prebuild fetch conditional and non-fatal when Electron is absent.

Suggested fix:

function resolveElectronVersion() {
  try {
    return require('electron/package.json').version;
  } catch {
    return null;
  }
}

return path.join(path.dirname(pkgJson), 'build', 'Release', filename);
}

function openDatabase(filename: string, options?: BetterSqlite3.Options): Database {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

openDatabase() now depends on a computed nativeBinding path, but failures here currently bubble up without additional context. Wrapping this call with a contextual error improves diagnosability and aligns with the project rule to throw with context.

Suggested fix:

function openDatabase(filename: string, options?: BetterSqlite3.Options): Database {
  const Database = require('better-sqlite3') as typeof BetterSqlite3;
  const nativeBinding = resolveNativeBinding();
  try {
    return new Database(filename, { ...options, nativeBinding });
  } catch (cause) {
    throw new Error(`Failed to open SQLite database with native binding (${nativeBinding})`, { cause: cause instanceof Error ? cause : new Error(String(cause)) });
  }
}

…stinstall

Prod-only installs (npm install --omit=dev, certain CI bootstraps,
end-user installer build steps) skip devDependencies, so the
electron module may not be present when postinstall runs. Wrap the
electron version lookup in try/catch and skip the Electron native
binding stage with a warning instead of hard-failing. The Node ABI
binding still installs so vitest and ad-hoc node consumers keep
working.
@hqhq1025
Copy link
Copy Markdown
Collaborator Author

Addressing Codex [Major] on apps/desktop/scripts/install-sqlite-bindings.cjs:43: resolveElectronVersion() now wraps the require in try/catch and returns null when electron is absent (prod-only npm install --omit=dev, lean CI bootstraps, end-user installer build steps). The Node-ABI binding still installs; the Electron stage is skipped with a warn log. Verified locally by moving apps/desktop/node_modules/electron aside and re-running the script — exits 0. Commit ed162a6.

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • No new high-confidence issues found in the added/modified lines of this update.

Summary

  • Review mode: follow-up after new commits
  • Previously reported install regression risk in apps/desktop/scripts/install-sqlite-bindings.cjs is addressed in the latest head.
  • Not found in repo/docs: docs/VISION.md, docs/PRINCIPLES.md.
  • Residual risk/testing gap: this PR still has no repo-added automated test covering runtime selection of nativeBinding (Node vs Electron) and postinstall behavior when Electron is absent.

Testing

  • Not run (automation)
  • Suggested tests: Vitest for resolveNativeBinding() runtime dispatch and an installer behavior test/path check for Electron-absent environments.

open-codesign Bot

@hqhq1025 hqhq1025 merged commit 33ca9fc into main Apr 19, 2026
5 of 6 checks passed
@hqhq1025 hqhq1025 deleted the fix/sqlite-abi-dual-target branch April 19, 2026 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant