feat(rivetkit): add json protocol support #3432
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
Code Review: JSON Protocol SupportThis PR adds JSON protocol support to RivetKit by introducing a flexible serialization system that can handle JSON, CBOR, and BARE encodings. Overall, this is a well-structured change with good architectural decisions. Here's my detailed feedback: Strengths ✅
Issues & Concerns
|
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: |
NathanFlurry
force-pushed
the
11-09-chore_rivetkit_add_required_zod_validation_for_json_encoding
branch
from
d27a9f0 to
be3dc7d
Compare
NathanFlurry
force-pushed
the
11-09-feat_rivetkit_add_json_protocol_support
branch
from
5f6c780 to
c713cc8
Compare
PR Review: Add JSON Protocol Support to RivetKitSummaryThis PR adds JSON encoding support to RivetKit's actor protocol, enabling communication using JSON alongside the existing BARE and CBOR encodings. This is a significant architectural change that affects the entire message serialization/deserialization pipeline. ✅ Strengths1. Well-Architected Abstraction 2. Comprehensive JSON Compatibility Layer 3. Consistent Pattern Application 🔍 Potential Issues & ConcernsCritical: Type Safety RegressionLocation: Multiple files // Before (type-safe):
sendMessage?(
actor: AnyActorInstance,
conn: AnyConn,
message: CachedSerializer<protocol.ToClient>,
): void;
// After (loosely typed):
sendMessage?(
actor: AnyActorInstance,
conn: AnyConn,
message: CachedSerializer<any, any, any>,
): void;Issue: Replacing specific types with Recommendation: Consider using generic constraints or union types instead of // Option 1: Generic with constraints
sendMessage?<TBare, TJson, T = TBare>(
actor: AnyActorInstance,
conn: AnyConn,
message: CachedSerializer<TBare, TJson, T>,
): void;
// Option 2: Union of known types
type AnyCachedSerializer =
| CachedSerializer<protocol.ToClient, ToClientJson, any>
| CachedSerializer</* other valid types */>;Bug: Potential Runtime Error in Error HandlingLocation: rivetkit-typescript/packages/rivetkit/src/actor/protocol/old.ts:296 const errorData = { group, code, message, metadata, actionId };
// ...
metadata: value.metadata ?? null, // In toJson
metadata: value.metadata
? bufferToArrayBuffer(cbor.encode(value.metadata))
: null, // In toBareIssue: The Recommendation: Initialize metadata explicitly: const errorData = {
group,
code,
message,
metadata: metadata ?? null, // Normalize undefined to null
actionId
};Performance: Unnecessary Double Serialization with CBORLocation: rivetkit-typescript/packages/rivetkit/src/serde.ts:68-71 } else if (encoding === "cbor") {
const jsonValue = toJson(value);
const validated = zodSchema.parse(jsonValue);
return cbor.encode(validated);Issue: For CBOR encoding, the code transforms to JSON format first, validates it, then CBOR-encodes it. This is inefficient because:
Recommendation: CBOR should use the BARE format transformation: } else if (encoding === "cbor") {
const bareValue = toBare(value);
// Optionally validate against a BARE schema if available
return cbor.encode(bareValue);This would be more efficient and maintain consistency (CBOR and BARE use the same underlying data types). Type Consistency: Mixed Message Type DefinitionsLocation: Multiple files (e.g., rivetkit-typescript/packages/rivetkit/src/actor/instance/mod.ts:514) async processMessage(
message: {
body:
| { tag: "ActionRequest"; val: { id: bigint; name: string; args: unknown } }
| { tag: "SubscriptionRequest"; val: { eventName: string; subscribe: boolean } };
},
// ...Issue: The PR introduces inline type definitions for messages instead of importing from the protocol definition. This creates multiple sources of truth and makes the codebase harder to maintain. Recommendation: Create and import a shared intermediate type: // In a shared types file
export type ProcessedMessage = {
body: ToServerBody; // Reuse from protocol-zod
};
// Usage
async processMessage(message: ProcessedMessage, conn: Conn<...>) {🔒 Security Considerations1. Input Validation ✅ Good 2. JSON Parsing Error Handling throw new Error(
`Unknown JSON encoding type: ${value[0]}. This may indicate corrupted data or a version mismatch.`,
);✅ Good defensive programming - unknown special types are rejected rather than silently ignored. 3. Base64 Encoding for Binary Data 📊 Performance ImplicationsPositive:
Concerns:
Recommendation: Document the performance trade-offs in the PR description or code comments so users can make informed decisions about which encoding to use. 🧪 Test CoverageExcellent:
Missing:
Recommendation: Add integration tests that: test("should handle action request/response roundtrip with JSON encoding", async () => {
// Create actor with JSON encoding
// Send action request
// Verify response
});
test("should produce equivalent results across all encodings", () => {
const testData = { /* complex test data */ };
const jsonResult = serializeWithEncoding("json", ...);
const cborResult = serializeWithEncoding("cbor", ...);
const bareResult = serializeWithEncoding("bare", ...);
// All should deserialize to equivalent data
});📝 Additional Recommendations
Summary VerdictThis is a well-architected feature with excellent test coverage for the core JSON compatibility layer. However, there are several concerns that should be addressed: Must Fix:
Should Fix:
Nice to Have:
Overall, the code quality is good, but I recommend addressing at least the "Must Fix" items before merging. |
PR Review: Add JSON Protocol SupportSummaryThis PR adds JSON encoding support to the RivetKit protocol alongside the existing BARE and CBOR encodings. The implementation introduces a sophisticated serialization layer that allows seamless switching between protocols while maintaining type safety. Architecture OverviewThe core innovation is the
Code Quality: ✅ StrongPositives:
Areas for Improvement:1. Type Safety ConcernsLocation: Multiple files (e.g., sendMessage?(
actor: AnyActorInstance,
conn: AnyConn,
message: CachedSerializer<any, any, any>, // ⚠️ Lost type safety
): void;Issue: Changing from Recommendation: Consider using a union type or generic constraint: message: CachedSerializer<protocol.ToClient, ToClientJson, unknown>2. Inline Type DefinitionsLocation: The PR introduces many inline type definitions for message structures instead of using the defined protocol types: message: {
body:
| {
tag: "ActionRequest";
val: { id: bigint; name: string; args: unknown };
}
| {
tag: "SubscriptionRequest";
val: { eventName: string; subscribe: boolean };
};
}Issue:
Recommendation: Create intermediate protocol types that represent the "decoded" versions: // In a shared protocol file
export type DecodedActionRequest = {
id: bigint;
name: string;
args: unknown; // Already decoded from ArrayBuffer/CBOR
};
export type DecodedToServer = {
body:
| { tag: "ActionRequest"; val: DecodedActionRequest }
| { tag: "SubscriptionRequest"; val: SubscriptionRequest };
};3. Error Handling - Incomplete Metadata HandlingLocation: metadata: value.metadata ?? null, // JSON pathvs metadata: value.metadata
? bufferToArrayBuffer(cbor.encode(value.metadata))
: null, // BARE pathQuestion: What happens if 4. Missing Edge Case ValidationLocation: } else if (encoding === "cbor") {
const decoded: unknown = cbor.decode(buffer);
const validated = zodSchema.parse(decoded);
return fromJson(validated); // ⚠️ Assumes CBOR and JSON schemas are identical
}Issue: The code assumes that CBOR-decoded data will match the JSON schema. While this is likely true for most cases, there could be edge cases where CBOR and JSON representations differ (e.g., how they handle special numeric values, undefined, etc.). Recommendation: Add comments documenting this assumption or consider separate validation paths. Security Considerations: ✅ Good
Minor concern: The JSON compatibility layer's error message at throw new Error(
`Unknown JSON encoding type: ${value[0]}. This may indicate corrupted data or a version mismatch.`,
);Consider whether this is acceptable or if it should be more generic. Performance Considerations: ✅ Excellent
Potential optimization: The Test Coverage:
|
NathanFlurry
force-pushed
the
11-09-feat_rivetkit_add_json_protocol_support
branch
from
c713cc8 to
412f83b
Compare
NathanFlurry
force-pushed
the
11-09-chore_rivetkit_add_required_zod_validation_for_json_encoding
branch
from
be3dc7d to
1784d8f
Compare
|
Deployment failed with the following error: Learn More: https://vercel.com/docs/limits#rate-limits |
Pull Request Review: JSON Protocol Support for RivetKitSummaryThis PR adds JSON protocol support to RivetKit's client-actor communication layer, enabling a more accessible and debuggable alternative to the existing BARE/CBOR binary protocols. The implementation introduces a flexible serialization system with encoding-specific transformations. StrengthsArchitecture & Design
Code Quality
Issues & Concerns1. Type Safety Regression (High Priority)Location: message: CachedSerializer<any, any, any>Issue: Using Recommendation: Define a union type for all possible message types: type ToClientSerializer =
| CachedSerializer<protocol.ToClient, ToClientJson, EventData>
| CachedSerializer<protocol.ToClient, ToClientJson, InitData>
| CachedSerializer<protocol.ToClient, ToClientJson, ErrorData>;
sendMessage?(
actor: AnyActorInstance,
conn: AnyConn,
message: ToClientSerializer,
): void;2. Inconsistent Type Annotations (Medium Priority)Location: Issue: Message type definitions are duplicated inline rather than importing from schema files. Example: message: {
body:
| { tag: "ActionRequest"; val: { id: bigint; name: string; args: unknown } }
| { tag: "SubscriptionRequest"; val: { eventName: string; subscribe: boolean } };
}Recommendation: Create a shared type definition: // In schemas/client-protocol-zod/mod.ts
export type ToServerInternal = {
body:
| { tag: "ActionRequest"; val: { id: bigint; name: string; args: unknown } }
| { tag: "SubscriptionRequest"; val: { eventName: string; subscribe: boolean } };
};3. Data Transformation Complexity (Medium Priority)Location: Issue: The dual transformation logic for JSON vs BARE protocols is complex and easy to get wrong. CBOR encoding happens in one path but not the other. Concern: Future maintainers might not understand when Recommendation: Add comprehensive documentation: /**
* Creates a CachedSerializer for event data that handles encoding differences:
*
* JSON Protocol: args remain as raw JavaScript values (arrays, objects, etc.)
* and are serialized directly by jsonStringifyCompat, which handles special
* types like BigInt and ArrayBuffer automatically.
*
* BARE/CBOR Protocol: args must be CBOR-encoded into an ArrayBuffer before
* being passed to the BARE serializer, as the protocol schema expects ArrayBuffer.
*/4. Schema Validation Performance (Low Priority)Location: Issue: Zod validation runs on every serialization, even for cached results. const validated = zodSchema.parse(jsonValue); // Could throw for invalid dataObservation: While validation is important for correctness, it adds overhead. Consider:
5. Missing Test Coverage (Medium Priority)Gaps identified:
Recommendation: Add tests like: describe("JSON Protocol Integration", () => {
test("should handle action request/response with JSON encoding", async () => {
// Test the full roundtrip with JSON encoding
});
test("should handle events with complex nested data in JSON", async () => {
// Test event broadcasting with JSON
});
});6. Error Handling Gaps (Low Priority)Location: Issue: The Recommendation: Add try-catch blocks with specific error messages for debugging: try {
return fromJson(validated);
} catch (err) {
throw new Error(`Failed to transform JSON message: ${err.message}`, { cause: err });
}Security ConsiderationsPositive
Consider
Performance ConsiderationsCaching StrategyThe
JSON vs CBOR Overhead
Best Practices & ConventionsFollows Repository Standards ✅
Suggestions
ConclusionThis is a well-architected PR that successfully adds JSON protocol support with a clean, extensible design. The main concerns are around type safety and test coverage, which should be addressed before merging. Recommendations Priority
Approval StatusThe core implementation is solid, but I recommend addressing the type safety issues before merging. Once the Estimated effort to address concerns: 2-3 hours 🤖 Generated with Claude Code |
Code Review: Add JSON Protocol SupportSummaryThis PR adds JSON protocol support to RivetKit, allowing it to work with JSON in addition to the existing BARE and CBOR binary encodings. The implementation introduces a flexible serialization system using a new Strengths✅ Well-structured architecture: The ✅ Comprehensive JSON compatibility layer: The ✅ Good test coverage: The ✅ Backward compatible: The changes maintain compatibility with existing BARE/CBOR encodings while adding JSON support. Issues & Concerns🔴 Critical: Type Safety DegradationLocation: Multiple files including // Before
sendMessage?(actor: AnyActorInstance, conn: AnyConn, message: CachedSerializer<protocol.ToClient>): void;
// After
sendMessage?(actor: AnyActorInstance, conn: AnyConn, message: CachedSerializer<any, any, any>): void;Problem: Using Recommendation: Define proper generic constraints or specific union types: type AnyCachedSerializer =
| CachedSerializer<protocol.ToClient, ToClientJson, any>
| CachedSerializer<protocol.ToServer, ToServerJson, any>;
sendMessage?(
actor: AnyActorInstance,
conn: AnyConn,
message: CachedSerializer<protocol.ToClient, ToClientJson, any>
): void;🟡 Medium: Inline Type Definitions Reduce ReusabilityLocation: Problem: Message type definitions are duplicated inline in multiple places rather than being defined as reusable types. This makes maintenance harder and increases the risk of inconsistencies. Example: async processMessage(
message: {
body:
| { tag: "ActionRequest"; val: { id: bigint; name: string; args: unknown }; }
| { tag: "SubscriptionRequest"; val: { eventName: string; subscribe: boolean }; };
},
conn: Conn<S, CP, CS, V, I, DB>,
)Recommendation: Extract these into named types in a shared location: // In schemas/client-protocol-zod/mod.ts or a new types file
export type ParsedToServer = {
body:
| { tag: "ActionRequest"; val: { id: bigint; name: string; args: unknown } }
| { tag: "SubscriptionRequest"; val: { eventName: string; subscribe: boolean } };
};
// Then use it
async processMessage(message: ParsedToServer, conn: Conn<S, CP, CS, V, I, DB>)🟡 Medium: Inconsistent Error HandlingLocation: if (\!versionedDataHandler) {
throw new Error("VersionedDataHandler is required for 'bare' encoding");
}Problem: Generic Recommendation: Use the RivetKit error system for consistency: import * as errors from "@/actor/errors";
if (\!versionedDataHandler) {
throw new errors.InternalError("VersionedDataHandler is required for 'bare' encoding");
}🟢 Minor: Magic Strings Without ConstantsLocation: Problem: The JSON encoding uses magic strings like Recommendation: Define constants for maintainability: const BIGINT_TYPE = "";
const ARRAYBUFFER_TYPE = "";
const UINT8ARRAY_TYPE = "";🟢 Minor: Commented-Out CodeLocation: Problem: Large blocks of commented-out code (serialize/deserialize functions) should be removed if no longer needed. Recommendation: Remove dead code or add a TODO comment explaining why it's preserved. Performance Considerations
Observation: The implementation at Security Considerations✅ Input validation: The use of Zod schemas for validation before processing is good security practice. Testing Recommendations
Documentation Suggestions
VerdictOverall Assessment: ✅ Approve with recommendations This is a solid implementation that successfully adds JSON protocol support. The architecture is well-designed with the Must Fix Before Merge:
Should Fix Soon:
Nice to Have:
Great work overall! The JSON compatibility layer is particularly well-implemented with proper handling of special types and edge cases. |
Merge activity
|
graphite-app
bot
deleted the
11-09-feat_rivetkit_add_json_protocol_support
branch
No description provided.