Skip to content

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

Merged
NathanFlurry merged 1 commit intomainfrom
03-23-fix_rivetkit_remove_drizzle-orm_peer_dep_and_add_runtime_version_check
Mar 25, 2026
Merged

fix(rivetkit): remove drizzle-orm peer dep and add runtime version check#4492
NathanFlurry merged 1 commit intomainfrom
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)
frontend-inspector ❌ Build Failed (View Logs) Web Mar 25, 2026 at 7:58 pm
kitchen-sink ❌ Build Failed (View Logs) Web Mar 25, 2026 at 7:58 pm
frontend-cloud 🕒 Building (View Logs) Web Mar 25, 2026 at 7:58 pm
website ❌ 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

@claude
Copy link

claude bot commented Mar 24, 2026

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

Overall this is a clean, focused change. The approach of removing the peer dep and adding a runtime warning is a good pattern for handling version compatibility. A few observations:

Correctness

test-drizzle-compat.sh - missing final newline in results table. The summary loop at the end has no trailing echo after the loop, so the last result line may get concatenated with the shell prompt in some terminals. Minor, but easy to fix by adding a closing echo after the for loop.

test-drizzle-compat.sh - set -e + cleanup trap interaction. The script uses set -euo pipefail. If pnpm install inside cleanup() fails on restore (e.g., network issue), the trap exits with an error and the calling process may see a misleading exit code. Consider adding || true to the restore pnpm install call - the goal of cleanup is best-effort restoration, not failure propagation.

Version check - createRequire is Node-specific. The node:module import works fine for Node.js but will throw in edge runtimes (Cloudflare Workers, Deno, Bun with strict mode, etc.). The try/catch already handles this gracefully, but a comment explaining why the catch is silent would help future maintainers understand it is intentional.

Design

DRIZZLE_MAX is exclusive but the constant name does not make that clear. The comment helps, but renaming to DRIZZLE_MAX_EXCLUSIVE or DRIZZLE_UPPER_BOUND would self-document the intent and prevent an off-by-one if someone edits the bounds later.

Module-level drizzleVersionChecked flag. Fine for typical usage, but because the flag is module-level it persists across tests in the same Node.js process. If a test wants to verify the warning fires, it will not unless the flag is reset. Not a blocker, just worth noting.

Style and Conventions

  • compareVersions is straightforward and readable.
  • Removing drizzle-orm from peerDependencies while keeping drizzle-kit is consistent - drizzle-kit is a build/migration-time tool, so keeping it as a peer dep makes sense.
  • The CLAUDE.md update is clear and useful for maintainers.
  • The formatting-only change to runInlineMigrations is fine.
  • The warning message correctly points to github.com/rivet-dev/rivet/issues per repo conventions.

Summary: The set -e/cleanup interaction in the shell script is the only item worth addressing before merge; the rest are low-priority suggestions.

Copy link
Member Author

NathanFlurry commented Mar 25, 2026

Merge activity

  • Mar 25, 7:43 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Mar 25, 7:58 PM UTC: Graphite rebased this pull request as part of a merge.
  • Mar 25, 7:59 PM UTC: @NathanFlurry merged this pull request with Graphite.

@NathanFlurry NathanFlurry changed the base branch from 03-23-chore_skill_add_version_check_to_rivetkit_skill to graphite-base/4492 March 25, 2026 19:54
@NathanFlurry NathanFlurry changed the base branch from graphite-base/4492 to main March 25, 2026 19:56
@NathanFlurry NathanFlurry force-pushed the 03-23-fix_rivetkit_remove_drizzle-orm_peer_dep_and_add_runtime_version_check branch from 2f7fec2 to 37824e5 Compare March 25, 2026 19:57
@NathanFlurry NathanFlurry merged commit e89e5eb into main Mar 25, 2026
11 of 20 checks passed
@NathanFlurry NathanFlurry deleted the 03-23-fix_rivetkit_remove_drizzle-orm_peer_dep_and_add_runtime_version_check branch March 25, 2026 19:59
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