Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Nov 3, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Ready Ready Preview Comment Nov 4, 2025 7:00pm
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 4, 2025 7:00pm
rivet-inspector Ignored Ignored Preview Nov 4, 2025 7:00pm
rivet-site Ignored Ignored Preview Nov 4, 2025 7:00pm

Copy link
Member Author

NathanFlurry commented Nov 3, 2025

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 Nov 3, 2025

PR Review: Set Minimum Node Version to 20

Summary

This PR adds a minimum Node.js version requirement of 20 to the root package.json. While this is a good practice for ensuring consistency, there are several inconsistencies across the codebase that should be addressed.

Issues Found

Critical: Version Inconsistencies Across Package.json Files

The codebase has conflicting Node.js version requirements:

  1. Root package.json (this PR): >=20
  2. rivetkit-typescript/packages/rivetkit/package.json:148: >=22.0.0
  3. frontend/packages/icons/package.json:15: >=18
  4. engine/docker/template/package.json:21: >=18.0.0

Warning: Documentation Inconsistencies

Examples documentation has varying Node.js requirements:

  • Most examples: Node.js 18+
  • examples/background-jobs: Node.js 20+
  • examples/bots: Node.js 22+

Recommendations

  1. Decide on a consistent minimum version for the entire monorepo

    • If Node 20 is the minimum, update all package.json files to match
    • If Node 22 is required (as rivetkit package suggests), update root to >=22
    • Consider whether sub-packages truly need different minimums
  2. Update CI/CD workflows: The GitHub workflows in .github/workflows/release.yaml use Node 20, which aligns with this PR but conflicts with the rivetkit package requirement

  3. Update documentation: Ensure all example READMEs reflect the chosen minimum version

  4. Consider adding .nvmrc: Add a .nvmrc file at the root to make it easier for developers to use the correct Node version

Code Quality

The change itself is syntactically correct and follows proper package.json format.

Suggested Next Steps

Before merging, please:

  1. Audit all Node.js version requirements across the codebase
  2. Standardize to a single minimum version (recommend 20 or 22)
  3. Update all package.json files, CI configs, and documentation to match
  4. Consider adding this check to CI to prevent future drift

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 3, 2025

More templates

@rivetkit/actor

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/actor@3310

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3310

@rivetkit/core

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/core@3310

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3310

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3310

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3310

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3310

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3310

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3310

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3310

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3310

commit: 33ba42a

@vercel
Copy link

vercel bot commented Nov 4, 2025

Deployment failed with the following error:

Too many requests - try again in 4 minutes (more than 120, code: "api-deployments-flood-pro").

Learn More: https://vercel.com/docs/limits#rate-limits

@claude
Copy link

claude bot commented Nov 4, 2025

Code Review

Summary

This PR sets the minimum Node.js version to 20 in the root package.json. While this is a reasonable change, there are significant inconsistencies across the codebase that should be addressed.

Issues Found

1. Inconsistent Node Version Requirements

The codebase has multiple conflicting Node version specifications:

  • Root package.json (this PR): >=20
  • frontend/packages/icons/package.json:15: >=18
  • engine/docker/template/package.json:21: >=18.0.0
  • rivetkit-typescript/packages/rivetkit/package.json:148: >=22.0.0

The rivetkit package requires Node 22+, which is HIGHER than the root requirement of 20.

2. CI/CD Configuration

GitHub Actions workflows use Node 20, which is consistent with this PR.

3. Docker Configuration

engine/sdks/typescript/test-runner/Dockerfile uses node:22-slim

4. Documentation Inconsistencies

Multiple example READMEs specify different Node requirements (18+, 20+, 22+)

Recommendations

  1. Decide on a single minimum Node version for the entire project
  2. Update all package.json files to use the same minimum version
  3. Update all example READMEs to reflect the chosen Node version
  4. Consider adding a .node-version or .nvmrc file to the root
  5. Verify rivetkit compatibility with Node 20 or update root to Node 22

Code Quality

The change itself is clean and follows correct package.json format.

Action Items

  • Resolve version inconsistencies across all package.json files
  • Update example documentation
  • Verify rivetkit compatibility with Node 20 or update root to >=22

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 4, 2025

Merge activity

  • Nov 4, 8:09 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 4, 8:11 PM UTC: CI is running for this pull request on a draft pull request (#3349) due to your merge queue CI optimization settings.
  • Nov 4, 8:41 PM UTC: The Graphite merge queue removed this pull request due to removal of a downstack PR #3342.
  • Nov 4, 8:49 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 4, 8:50 PM UTC: CI is running for this pull request on a draft pull request (#3352) due to your merge queue CI optimization settings.
  • Nov 4, 8:52 PM UTC: Merged by the Graphite merge queue via draft PR: #3352.

graphite-app bot pushed a commit that referenced this pull request Nov 4, 2025
@NathanFlurry NathanFlurry force-pushed the 10-31-fix_fe_show_connect_page_when_there_are_no_runner_configs_but_runner_names branch from f51b221 to d5c5825 Compare November 4, 2025 20:25
@NathanFlurry NathanFlurry force-pushed the 11-02-chore_set_minimum_node_version_to_20 branch from 141efff to 33ba42a Compare November 4, 2025 20:25
@vercel
Copy link

vercel bot commented Nov 4, 2025

Deployment failed with the following error:

Too many requests - try again in 5 minutes (more than 120, code: "api-deployments-flood-pro").

Learn More: https://vercel.com/docs/limits#rate-limits

@claude
Copy link

claude bot commented Nov 4, 2025

Code Review

Summary

This PR sets the minimum Node.js version to 20 in the root workspace package.json. This is a reasonable change given that Node 18 reaches end-of-life in April 2025.

Findings

1. Inconsistent Node Version Requirements Across Workspace ⚠️

The workspace currently has inconsistent Node version requirements across different packages:

  • Root workspace (this PR): >=20
  • rivetkit package: >=22.0.0 (most restrictive)
  • @rivet-gg/icons: >=18 (least restrictive)
  • docker/template: >=18.0.0

Recommendation: For a monorepo, all packages should align on a consistent minimum Node version. I suggest one of the following approaches:

  1. Option A (Recommended): Align all packages to Node 20+ to match this PR

    • Update frontend/packages/icons/package.json from >=18 to >=20
    • Update engine/docker/template/package.json from >=18.0.0 to >=20
    • Consider whether rivetkit truly requires Node 22, or if it can be lowered to 20 for consistency
  2. Option B: If rivetkit genuinely needs Node 22 features, make that the minimum across all packages

Rationale: Having different Node requirements can lead to:

  • Confusion for contributors about which Node version to use
  • Potential runtime errors if a package uses features not available in the minimum version
  • CI/CD complications

2. Node Version Support Strategy 📋

Consider documenting the Node version support policy:

  • Will you support only Active LTS releases?
  • What's the deprecation/upgrade timeline when new LTS versions are released?
  • Should this be documented in a CONTRIBUTING.md or similar?

Additional Observations

Good:

  • The syntax >=20 is correct and follows npm conventions
  • Setting this at the workspace root will warn users during pnpm install if they're using an incompatible version
  • Timing is appropriate given Node 18 EOL

Testing Recommendations

  • Verify CI/CD pipelines are using Node 20+ (check .github/workflows/ files if they specify node-version)
  • Test that pnpm install properly warns/errors with Node 18 or earlier
  • Ensure all examples and documentation reference Node 20+ as minimum

Verdict

Approve with suggestions. The change itself is sound, but addressing the version inconsistencies across the workspace would prevent potential issues for developers and users.

graphite-app bot pushed a commit that referenced this pull request Nov 4, 2025
@graphite-app graphite-app bot closed this Nov 4, 2025
@graphite-app graphite-app bot deleted the 11-02-chore_set_minimum_node_version_to_20 branch November 4, 2025 20:52
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.

3 participants