Skip to content

fix(rivetkit): remove drizzle-orm peer dep and add runtime version check#4492

Draft
NathanFlurry wants to merge 1 commit into03-23-chore_skill_add_version_check_to_rivetkit_skillfrom
03-23-fix_rivetkit_remove_drizzle-orm_peer_dep_and_add_runtime_version_check
Draft

fix(rivetkit): remove drizzle-orm peer dep and add runtime version check#4492
NathanFlurry wants to merge 1 commit into03-23-chore_skill_add_version_check_to_rivetkit_skillfrom
03-23-fix_rivetkit_remove_drizzle-orm_peer_dep_and_add_runtime_version_check

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

@railway-app
Copy link

railway-app bot commented Mar 24, 2026

🚅 Deployed to the rivet-pr-4492 environment in rivet-frontend

Service Status Web Updated (UTC)
website ❌ Build Failed (View Logs) Web Mar 24, 2026 at 3:29 am
frontend-inspector ❌ Build Failed (View Logs) Web Mar 24, 2026 at 3:29 am
frontend-cloud ❌ Build Failed (View Logs) Web Mar 24, 2026 at 3:29 am
kitchen-sink ❌ Build Failed (View Logs) Web Mar 24, 2026 at 3:29 am
mcp-hub ✅ Success (View Logs) Web Mar 24, 2026 at 3:29 am
ladle ❌ Build Failed (View Logs) Web Mar 24, 2026 at 3:29 am

Copy link
Member Author

@claude
Copy link

claude bot commented Mar 24, 2026

PR Review: fix(rivetkit): remove drizzle-orm peer dep and add runtime version check

Good approach overall. Removing the pinned peer dependency while adding a soft runtime warning is cleaner than forcing users to match a specific drizzle-orm version. A few issues worth addressing before merge:

Bugs / Correctness

1. CLAUDE.md references a non-existent symbol

The new CLAUDE.md section says:

Update the DEFAULT_VERSIONS array in the script and SUPPORTED_DRIZZLE_RANGE in packages/rivetkit/src/db/drizzle/mod.ts...

But mod.ts uses DRIZZLE_MIN and DRIZZLE_MAX, not SUPPORTED_DRIZZLE_RANGE. This will confuse future maintainers searching for that name. Update the doc to reference the actual constant names.

2. declare -A requires Bash 4+, but macOS ships Bash 3.2

rivetkit-typescript/packages/rivetkit/scripts/test-drizzle-compat.sh line 37 uses declare -A RESULTS.

macOS ships with Bash 3.2 (GPL v2), which does not support associative arrays. Any macOS developer running this script with the system Bash will get a cryptic error. Consider either adding a Bash version guard at the top, documenting that Homebrew Bash 5+ is required, or rewriting the results summary using parallel indexed arrays.

Minor Issues

3. DRIZZLE_MAX will need updating as new versions are tested

DRIZZLE_MAX = [0, 46, 0] (exclusive) means any 0.46.x+ release will trigger the warning immediately on release. This is intentional and correct, but the automation (CLAUDE.md + the script) only tests up to 0.45. When 0.46 ships, someone will need to add 0.46 to DEFAULT_VERSIONS in the script and bump DRIZZLE_MAX to [0, 47, 0]. This workflow is described in CLAUDE.md, which is good. Just worth calling out explicitly in the review.

4. Cleanup fallback pnpm install without --frozen-lockfile on failure

test-drizzle-compat.sh:45 has: cd "$ROOT_DIR" && pnpm install --frozen-lockfile 2>/dev/null || pnpm install

The mv restores the original lockfile before this runs, so --frozen-lockfile should succeed. The fallback is a safety net, but it suppresses errors from the first attempt and then freely re-resolves deps, potentially leaving node_modules in an inconsistent state. Consider printing a warning when the fallback fires.

5. drizzle-kit is still declared as an optional peer dep but drizzle-orm is not

Users who add drizzle integration will need both drizzle-orm (for queries) and drizzle-kit (for migrations). With this PR, drizzle-kit remains an optional peer dep but drizzle-orm is removed entirely. Consider whether the companion drizzle-kit entry should also be removed for consistency, or add a comment in package.json explaining why one is kept and the other is not.

Nits

  • isSupported: The NaN case from .map(Number) is not explicitly handled. If a version string somehow passes the stripping but still contains non-numeric segments, compareVersions will silently produce NaN-based comparisons. The try/catch in checkDrizzleVersion makes this safe in practice, but an explicit isNaN guard in isSupported would be more robust.
  • If cd into PKG_DIR itself fails, the error propagates and set -e exits before cleanup. Not a real-world concern but worth noting.

Summary

The core design is solid. The main actionable items are:

  1. Fix the SUPPORTED_DRIZZLE_RANGE -> DRIZZLE_MIN/DRIZZLE_MAX doc mismatch
  2. Handle the Bash 4+ requirement for declare -A on macOS

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