Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@railway-app
Copy link

railway-app bot commented Jan 8, 2026

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

Service Status Web Updated (UTC)
frontend-cloud-staging 🕒 Building (View Logs) Web Jan 9, 2026 at 9:23 pm
frontend-cloud 🕒 Building (View Logs) Web Jan 9, 2026 at 9:23 pm
website 🕐 Queued (View Logs) Web Jan 9, 2026 at 9:22 pm
frontend-inspector 🕐 Queued (View Logs) Web Jan 9, 2026 at 9:22 pm

🚅 Deployed to the rivet-pr-3788 environment in rivet-frontend-website-preview

Service Status Web Updated (UTC)
frontend-cloud ◻️ Removed (View Logs) Jan 9, 2026 at 5:01 pm

@railway-app railway-app bot temporarily deployed to rivet-frontend-website-preview / rivet-pr-3788 January 8, 2026 00:13 Destroyed
@railway-app railway-app bot temporarily deployed to rivet-frontend / rivet-pr-3788 January 8, 2026 00:13 Destroyed
Copy link
Member Author

NathanFlurry commented Jan 8, 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 Jan 8, 2026

Pull Request Review

Summary

This PR adds @hono/node-server and @hono/node-ws as devDependencies to all examples in the repository. The changes move these packages from dependencies to devDependencies where they were already present, and add them to examples that did not have them before.

Code Quality ✅

Positive aspects:

  • Consistent version pinning across most examples (^1.19.7 for node-server, ^1.3.0 for node-ws)
  • Properly placed in devDependencies rather than dependencies
  • Clean lockfile updates with no unexpected changes

Issues identified:

1. ⚠️ Unnecessary dependencies added to non-Hono examples

Several examples do not use Hono at all but still received these dependencies:

  • express (examples/express/package.json:13-14) - Uses Express, not Hono
  • elysia (examples/elysia/package.json:13-14) - Uses Elysia, not Hono
  • deno (examples/deno/package.json:13-14) - Uses Deno native server, not Hono
  • trpc (examples/trpc/package.json:14-15) - Uses tRPC

These examples do not import or use @hono/node-server or @hono/node-ws in their code. Adding unused dependencies:

  • Increases install time and node_modules size
  • Creates confusion about the example actual dependencies
  • Violates the principle of minimal dependencies

Recommendation: Remove these dependencies from examples that do not use Hono.

2. ⚠️ Version inconsistencies

Most examples use ^1.19.7 for @hono/node-server, but some use ^1.19.1:

  • examples/hono/package.json:14
  • examples/hono-react/package.json:31
  • examples/ai-and-user-generated-actors-freestyle/package.json:30
  • examples/raw-fetch-handler/package.json:18
  • examples/raw-websocket-handler-proxy/package.json:18

While this is not critical (all resolve to compatible versions), consistency across the monorepo is better for maintenance.

Recommendation: Standardize to ^1.19.7 across all examples.

3. ⚠️ Missing dependency usage verification

The PR adds @hono/node-ws to all examples, but a codebase search shows only one example actually uses it:

  • examples/raw-websocket-handler-proxy/src/server.ts:1

Most examples do not need WebSocket support. Adding @hono/node-ws unnecessarily:

  • Adds extra size to node_modules
  • Creates false expectations about WebSocket functionality
  • Requires maintenance for security updates despite not being used

Recommendation: Only add @hono/node-ws to examples that actually use WebSockets.

Performance Considerations ✅

No significant performance impact expected. The dependencies are development-time only and do not affect runtime performance.

Security Concerns ✅

  • Dependencies are from the official Hono organization
  • Version ranges use caret which is standard for non-critical packages
  • No known vulnerabilities in the specified versions

Test Coverage ⚠️

Missing:

  • No indication that examples were tested after adding these dependencies
  • Examples that do not use these packages will not fail even if the deps are broken

Recommendation: Run at minimum pnpm install and pnpm build for all examples to verify the changes do not break anything.

Specific File Issues

examples/chat-room/package.json:16
The start script path changed from --static=dist to --static=frontend/dist. This seems unrelated to adding node-server dependencies. Was this an intentional fix or an accidental change?

examples/hono/package.json:21
Duplicate @hono/node-server dependency removed from dependencies section - good cleanup.

Recommendations Summary

High Priority:

  1. Remove @hono/node-server and @hono/node-ws from examples that do not use Hono (express, elysia, deno, trpc, etc.)
  2. Only add @hono/node-ws to examples that actually use WebSockets

Medium Priority:
3. Standardize @hono/node-server version to ^1.19.7 across all examples that need it
4. Verify the chat-room start script path change was intentional
5. Test that all examples build and run successfully

Low Priority:
6. Consider documenting why these dependencies are needed in examples that use registry.serve() (which might internally use these packages)

Verdict

The intent is good - standardizing dependencies across examples. However, the implementation is too broad, adding dependencies to examples that do not need them. The PR would benefit from a more targeted approach that only adds dependencies where they are actually used.

Status: ⚠️ Changes Requested

@vercel
Copy link

vercel bot commented Jan 9, 2026

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

Project Deployment Review Updated (UTC)
rivet-cloud Ready Ready Preview, Comment Jan 9, 2026 9:25pm
rivetkit-serverless Error Error Jan 9, 2026 9:25pm
2 Skipped Deployments
Project Deployment Review Updated (UTC)
rivet-inspector Ignored Ignored Preview Jan 9, 2026 9:25pm
rivet-site Ignored Ignored Preview Jan 9, 2026 9:25pm

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jan 9, 2026

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@3788

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 1fec8fb

@NathanFlurry NathanFlurry force-pushed the 01-07-chore_examples_add_node_server_dependency branch from 1fec8fb to 679877f Compare January 9, 2026 21:22
@railway-app railway-app bot temporarily deployed to rivet-frontend / rivet-pr-3788 January 9, 2026 21:22 Destroyed
@graphite-app
Copy link
Contributor

graphite-app bot commented Jan 9, 2026

Merge activity

  • Jan 9, 9:23 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Jan 9, 9:23 PM UTC: CI is running for this pull request on a draft pull request (#3804) due to your merge queue CI optimization settings.
  • Jan 9, 9:24 PM UTC: Merged by the Graphite merge queue via draft PR: #3804.

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