-
Notifications
You must be signed in to change notification settings - Fork 130
refactor(rivetkit): restructure connection lifecycle contexts #3428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: graphite-base/3428
Are you sure you want to change the base?
refactor(rivetkit): restructure connection lifecycle contexts #3428
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
4b4b76b to
aedd8f2
Compare
More templates
@rivetkit/actor
@rivetkit/cloudflare-workers
@rivetkit/core
@rivetkit/db
@rivetkit/framework-base
@rivetkit/next-js
@rivetkit/react
rivetkit
@rivetkit/sql-loader
@rivetkit/engine-runner
@rivetkit/engine-runner-protocol
commit: |
Code Review: Connection Lifecycle Context RestructuringSummaryThis PR refactors the connection lifecycle context handling in RivetKit by:
Positive AspectsArchitecture & Design✅ Excellent use of inheritance hierarchy: The new ConnInitContext and ConnContext base classes create a clear and logical separation between connection initialization phases and active connection phases. Context hierarchy:
✅ Cleaner API: Moving request from an opts parameter to a context property (c.request) is more ergonomic and consistent with the rest of the API. ✅ Type safety: The request?: Request | undefined typing correctly reflects that requests may not always be present in all connection scenarios. Implementation Quality✅ Consistent refactoring: All lifecycle hooks have been updated consistently across the codebase. ✅ Backward compatibility considerations: The migration path is clear, as evidenced by the fixture updates. Issues & Concerns🔴 Critical: Incomplete Migration (Blocking)Location: rivetkit-typescript/packages/rivetkit/fixtures/driver-test-suite/raw-websocket.ts:54 The getRequestInfo handler is commented out with a throw "TODO" statement. This is a runtime bomb that will fail if any test calls this code path. Recommendation: Either:
Suggested fix: Access ctx.request to get the request URL info, similar to how it's done in request-access.ts fixture.
|
Code Review: Connection Lifecycle Context RestructuringThis PR refactors the RivetKit connection lifecycle contexts to improve type safety and reduce code duplication. Overall, the changes are well-structured and follow good architectural patterns. Strengths
Issues and Concerns1. Breaking Change with TODO Comment (HIGH PRIORITY) In rivetkit-typescript/packages/rivetkit/fixtures/driver-test-suite/raw-websocket.ts:54, the code now has throw TODO where request info should be returned. This test fixture now throws an error instead of providing working functionality. Recommendation: Either complete the implementation using c.request, remove this test case if no longer relevant, or add a proper error message. 2. Request Availability Not Clear (MEDIUM PRIORITY) The request field is marked as Request | undefined in multiple contexts, but the documentation does not clearly explain when it will be undefined and what users should check before using it. Recommendation: Add clearer documentation explaining that request will be defined for connections initiated via HTTP/WebSocket upgrade requests, but undefined for connections restored from hibernation. 3. Empty Type Definition (LOW PRIORITY) In websocket.ts:8, changed from empty object to Record never never. While more explicit, the old empty object was clearer for representing an empty object type. 4. Type Safety in ConnectionManager (LOW PRIORITY) In connection-manager.ts, there are several as any casts at lines 153, 275, and 286. Consider adding proper type definitions to avoid type assertions. Questions
Performance and Security
Recommendations SummaryBefore Merge:
Nice to Have: Overall AssessmentThis is a solid refactoring that improves code quality and API design. The inheritance hierarchy is well-thought-out and the reduction in code duplication is significant. The main blocker is the incomplete TODO implementation. Recommendation: Request changes for the TODO issue, then approve. |

No description provided.