feat(client, server)!: update event-source errors and decode behavior #182
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis update modifies event payload formatting in test files by appending a double newline to the event strings for "done" and "error" events. The tests for the event source in both the fetch and node packages have been updated accordingly. Additionally, the test cases for the EventDecoder and EventDecoderStream have been restructured to include more scenarios, particularly for incomplete messages. The error handling in the OpenAPISerializer and RPCSerializer classes has been simplified by removing specific checks for Changes
Sequence Diagram(s)sequenceDiagram
participant F as Event Feed
participant D as EventDecoder
participant C as Callback
F->>D: feed("event data\n\n")
D->>C: onEvent(complete event)
F->>D: feed("incomplete event data")
D-->>D: Accumulate incomplete segment
D->>D: end() called
D-->>F: throw EventDecoderError (if incomplete message remains)
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (10)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (19)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
More templates
@orpc/client
@orpc/contract
@orpc/openapi
@orpc/react-query
@orpc/server
@orpc/shared
@orpc/standard-server
@orpc/standard-server-node
@orpc/standard-server-fetch
@orpc/vue-colada
@orpc/vue-query
@orpc/zod
commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/standard-server-fetch/src/event-source.test.ts(3 hunks)packages/standard-server-node/src/event-source.test.ts(3 hunks)packages/standard-server/src/event-source/decoder.test.ts(1 hunks)packages/standard-server/src/event-source/decoder.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/standard-server-fetch/src/event-source.test.ts
- packages/standard-server-node/src/event-source.test.ts
🔇 Additional comments (5)
packages/standard-server/src/event-source/decoder.ts (1)
89-92: Added error handling for incomplete event streamsThis change addresses the core issue in the PR. Previously, the code attempted to force-complete the message by adding newlines. Now it correctly throws an error when an event source ends prematurely with incomplete data, making the behavior more predictable and explicit.
packages/standard-server/src/event-source/decoder.test.ts (4)
68-108: Improved test structure for successful event decodingThe test case has been restructured to provide better clarity by separating success scenarios. The test now explicitly verifies that the decoder properly handles multiple complete messages including when parts of a message arrive in separate chunks.
110-127: Added test coverage for incomplete message handlingThis new test case is crucial for validating the core fix introduced in this PR. It properly verifies that the decoder throws an error when an incomplete message is encountered at the end of the stream while still correctly processing any complete messages that came before.
130-182: Restructured stream decoder tests for successful processingThe test implementation now more thoroughly validates the stream decoder's handling of complete messages, ensuring that multiple messages are correctly decoded even when split across different stream chunks.
184-215: Added test coverage for stream decoder with incomplete messagesThis test case is essential for verifying that the stream decoder properly propagates errors from the underlying decoder when encountering incomplete messages, while still correctly handling any complete messages that came before the error.
| } | ||
|
|
||
| const completes = this.incomplete.slice(0, lastCompleteIndex + 2).split(/\n{2,}/) | ||
| const completes = this.incomplete.slice(0, lastCompleteIndex).split(/\n\n/) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactored incomplete message handling
The modification to use slice(0, lastCompleteIndex) before splitting ensures that only fully complete portions of the message string are processed, improving message boundary detection reliability.
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/client/src/rpc/serializer.test.ts (1)
180-193: Consider using a regular async function instead of a generator.The test correctly validates that an ErrorEvent is properly deserialized with its data. However, the static analysis tool flagged that the generator function doesn't contain any
yieldstatements. Since this function only throws an error without yielding any values, consider using a regular async function instead:it('deserialize an invalid-ORPCError', async () => { - const iterator = serializer.deserialize((async function* () { + const iterator = serializer.deserialize((async function () { throw new ErrorEvent({ data: { json: { value: 1234 } }, }) })()) as any await expect(iterator.next()).rejects.toSatisfy((e: any) => { expect(e).toBeInstanceOf(ErrorEvent) expect(e.data).toEqual({ value: 1234 }) return true }) })This change would eliminate the static analysis warning while maintaining the same test behavior.
🧰 Tools
🪛 Biome (1.9.4)
[error] 181-185: This generator function doesn't contain yield.
(lint/correctness/useYield)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/client/src/index.ts(1 hunks)packages/client/src/openapi/serializer.test.ts(0 hunks)packages/client/src/openapi/serializer.ts(0 hunks)packages/client/src/rpc/serializer.test.ts(1 hunks)packages/client/src/rpc/serializer.ts(0 hunks)
💤 Files with no reviewable changes (3)
- packages/client/src/rpc/serializer.ts
- packages/client/src/openapi/serializer.test.ts
- packages/client/src/openapi/serializer.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/client/src/rpc/serializer.test.ts
[error] 181-185: This generator function doesn't contain yield.
(lint/correctness/useYield)
🔇 Additional comments (1)
packages/client/src/index.ts (1)
10-11: Looks good - exposing ErrorEvent to clients.Adding this export makes ErrorEvent directly accessible to consumers of the client package, which aligns with the simplified error handling approach mentioned in the PR.
Summary by CodeRabbit
Bug Fixes
Tests
ErrorEventhandling and added new tests for deserialization of invalidORPCError.Refactor
OpenAPISerializerandRPCSerializerclasses for a more uniform approach.EventMessagestructure to include optionaldataand a newcommentsarray for better information handling.