Skip to content

fix(rivetkit): call importNodeDependencies() in drizzle db() to fix migration crypto error#4963

Closed
abcxff wants to merge 1 commit intorivetkit-db/onmigrate-savepointfrom
05-04-_slop_fix_rivetkit_call_importnodedependencies_in_drizzle_db_to_fix_migration_crypto_error
Closed

fix(rivetkit): call importNodeDependencies() in drizzle db() to fix migration crypto error#4963
abcxff wants to merge 1 commit intorivetkit-db/onmigrate-savepointfrom
05-04-_slop_fix_rivetkit_call_importnodedependencies_in_drizzle_db_to_fix_migration_crypto_error

Conversation

@abcxff
Copy link
Copy Markdown
Contributor

@abcxff abcxff commented May 4, 2026

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

This was referenced May 4, 2026
Copy link
Copy Markdown
Contributor Author

abcxff commented May 4, 2026

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

@abcxff abcxff mentioned this pull request May 4, 2026
11 tasks
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 4, 2026

Code Review

Summary

This PR adds a call to importNodeDependencies() at the top of the db() function in rivetkit-typescript/packages/rivetkit/src/db/drizzle.ts. The intent is to ensure the Node.js crypto module is loaded before getNodeCrypto() is called during migrations (which would otherwise throw with a confusing "Node crypto module not loaded" error).

The fix is correct and minimal. A few observations worth considering:


Issues

1. importNodeDependencies() is called at factory-definition time, not at migration time

db() returns a DatabaseProvider object — it does not run migrations or access crypto itself. The actual getNodeCrypto() call happens inside runMigrations(), which is invoked from the onMigrate callback on the returned provider object. This means importNodeDependencies() currently runs synchronously when the user calls db({ migrations: ... }) to define their database provider (module load time), not when the migration executes.

This is effectively fine in Node.js (where the function is cheap and idempotent), but it means errors from importNodeDependencies() surface at definition time rather than at migration time. If this provider is used in an environment where Node modules are unavailable (e.g. wasm/edge), calling db() will now throw immediately during module evaluation rather than failing gracefully at migration time.

Consider whether importNodeDependencies() should be deferred to createClient or onMigrate instead, so non-Node runtimes can still use the db() factory (even if they'll fail on onMigrate).

2. The PR description is not filled out

The PR body uses the default template with no description, no test plan, and no checklist items checked. Even for a small fix, it should describe the root cause (calling db() before registry.start() or any other importNodeDependencies() call site) and how it was tested.

3. [slop] prefix in the commit message and branch name

The commit message headline is [slop]fix(rivetkit): call importNodeDependencies() in drizzle db() to…. The [slop] prefix is non-standard and violates the project's conventional commit style. CLAUDE.md requires git commit -m "chore(my-pkg): foo bar" with no extra prefixes.


Observations (non-blocking)

importNodeDependencies() is already idempotent (hasImportedDependencies guard in node.ts), so calling it from db() adds no overhead on repeated calls. The fix is safe.

No other db()-like factory function needs this change. The only other file in src/db/ is mod.ts (re-exports), and the only other file using getNodeCrypto outside drizzle.ts is node.ts itself. The scope is correct.

The fix aligns with the CLAUDE.md "Fail-By-Default Runtime" principle — surfacing the missing dependency early rather than letting it crash deep inside migration logic.


Verdict

The code change itself is correct and minimal. The main concerns are:

  1. Consider deferring importNodeDependencies() to onMigrate so wasm/edge runtimes can still construct a db() provider without immediately throwing.
  2. The PR description and checklist need to be filled in.
  3. The [slop] commit message prefix should be removed before merge.

@NathanFlurry NathanFlurry changed the title [slop]fix(rivetkit): call importNodeDependencies() in drizzle db() to fix migration crypto error fix(rivetkit): call importNodeDependencies() in drizzle db() to fix migration crypto error May 5, 2026
NathanFlurry
NathanFlurry previously approved these changes May 5, 2026
@NathanFlurry NathanFlurry marked this pull request as ready for review May 5, 2026 11:23
@NathanFlurry NathanFlurry marked this pull request as draft May 5, 2026 11:45
Copy link
Copy Markdown
Member

#4971

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.

2 participants