Skip to content

chore(rivetkit): update wa-sqlite references in comments to @rivetkit/sqlite#4267

Closed
NathanFlurry wants to merge 1 commit into02-23-chore_rivetkit_replace_wa-sqlite_with_rivetkit_sqlitefrom
02-23-chore_rivetkit_update_wa-sqlite_references_in_comments_to_rivetkit_sqlite
Closed

chore(rivetkit): update wa-sqlite references in comments to @rivetkit/sqlite#4267
NathanFlurry wants to merge 1 commit into02-23-chore_rivetkit_replace_wa-sqlite_with_rivetkit_sqlitefrom
02-23-chore_rivetkit_update_wa-sqlite_references_in_comments_to_rivetkit_sqlite

Conversation

@NathanFlurry
Copy link
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Member Author

NathanFlurry commented Feb 23, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link

claude bot commented Feb 23, 2026

PR Review

This PR does two things: renames wa-sqlite references in comments/docs to @rivetkit/sqlite, and refactors the ActorDriver interface to replace the sqliteVfs property with a getSqliteVfs() method.

Code Changes

sqliteVfs property → getSqliteVfs() method

The refactor is well-motivated and the reasoning is clearly documented. Making this a method instead of a property enables drivers to use dynamic imports, which is the right approach for keeping the core driver bundle free of SQLite.

One subtle concern in actor/instance/mod.ts:

if (!this.#sqliteVfs && this.driver.getSqliteVfs) {
    this.#sqliteVfs = await this.driver.getSqliteVfs();
}

The old code (this.#sqliteVfs ??= this.driver.sqliteVfs) was a synchronous, atomic assignment. The new async version introduces a narrow race window: if #setupDatabase were somehow called concurrently before the first await resolves, two VFS instances could be created. In practice this shouldn't occur given normal actor lifecycle, but a guard like this.#sqliteVfsPromise ??= this.driver.getSqliteVfs(); this.#sqliteVfs = await this.#sqliteVfsPromise; would eliminate the window entirely.

Dynamic import concatenation in file-system/actor.ts

const specifier = "@rivetkit/" + "sqlite-vfs";
const { SqliteVfs } = await import(specifier);

The comment explaining why this is necessary (prevent wrangler/esbuild static analysis) is good. TypeScript loses type narrowing on the destructured SqliteVfs here (the dynamic specifier resolves to any), but the function's return type annotation (Promise<SqliteVfs>) from the import type at the top keeps the public API typed correctly. This is an acceptable tradeoff.

Singleton in db/sqlite-vfs.ts

This file exports a getSqliteVfs() helper that returns a cached singleton instance. The new ActorDriver interface JSDoc explicitly says drivers should return a new instance per call for actor-level isolation. These two are in tension — if an external consumer implements ActorDriver.getSqliteVfs by delegating to this helper, they'd silently break the isolation invariant. Consider adding a comment to sqlite-vfs.ts clarifying the contexts where a singleton is safe (e.g., Cloudflare Workers, where each actor runs in its own isolate) vs. when a fresh instance per call is required.

Breaking change

Removing sqliteVfs?: SqliteVfs from the interface is a breaking change for any external ActorDriver implementations that set this property. Since both the old and new members are optional, it's a soft break, but worth calling out in the PR description or a changelog note.

Comment/Documentation Updates

All wa-sqlite comment-level references have been updated to @rivetkit/sqlite. The underlying asset file references (wa-sqlite-async.mjs, wa-sqlite-async.wasm) are correctly left unchanged since those are internal file names within @rivetkit/sqlite.

The multiplayer-game.mdx condensation looks good — the content is restructured into cleaner tables and bullet points without losing correctness.

Adding "Codex" to the supported assistants list in [...slug].astro is accurate.

Summary

The rename is clean and complete. The substantive change (property → method) is sound. Main items worth addressing before merge:

  1. Consider guarding #setupDatabase against the concurrent-call race using a promise cache instead of a value cache.
  2. Add a clarifying comment to db/sqlite-vfs.ts explaining when the singleton pattern is safe vs. when a per-call fresh instance is required.
  3. Note the breaking change in the PR description.

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