Feature: implement streaming api reading in client#45
Conversation
There was a problem hiding this comment.
Pull request overview
Adds client-side support for consuming line-delimited JSON streaming responses end-to-end (ServiceClient → Requester → fetch stream reader).
Changes:
- Replaces constructor-based response parsing with a
ResponseDataClass.fromObjectstatic factory contract. - Adds streaming request APIs in
ServiceClientandRequester. - Implements streaming fetch handling that reads newline-delimited JSON items and invokes a callback per parsed item.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/services/serviceClient.ts | Introduces a static factory type for response parsing and adds a streaming request wrapper. |
| src/requester/requester.ts | Adds streaming request plumbing and implements streaming fetch + NDJSON parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .catch((error): void => { | ||
| throw new KibaException(`The request was made but no response was received: [${error.code}] "${error.message}"`); | ||
| }) | ||
| .then(async (response: Response | void): Promise<KibaResponse> => { | ||
| if (!response) { | ||
| throw new KibaException('The request was made but no response was received.'); | ||
| } |
There was a problem hiding this comment.
Chaining .catch(...).then(...) forces the then handler to accept Response | void, which complicates typing and requires an additional !response guard. Consider rewriting this section with try/catch around an awaited fetch(...) so response stays a Response, simplifying control flow and types.
| .catch((error): void => { | |
| throw new KibaException(`The request was made but no response was received: [${error.code}] "${error.message}"`); | |
| }) | |
| .then(async (response: Response | void): Promise<KibaResponse> => { | |
| if (!response) { | |
| throw new KibaException('The request was made but no response was received.'); | |
| } | |
| .catch((error): never => { | |
| throw new KibaException(`The request was made but no response was received: [${error.code}] "${error.message}"`); | |
| }) | |
| .then(async (response: Response): Promise<KibaResponse> => { |
| private makeStreamingFetchRequest = async <StreamItemType>( | ||
| request: KibaRequest, | ||
| parseItem: (obj: Record<string, unknown>) => StreamItemType, | ||
| onItem: (item: StreamItemType) => void, | ||
| ): Promise<KibaResponse> => { | ||
| const url = new URL(request.url); | ||
| const headers = new Headers({ ...this.headers, ...(request.headers || {}) }); |
There was a problem hiding this comment.
makeStreamingFetchRequest appears to duplicate substantial logic from the existing non-streaming fetch path (URL/header construction, method/body handling, status error handling). This duplication increases the risk of behavioral drift (e.g., a bugfix applied to one path but not the other). Consider extracting shared helpers (e.g., building fetchConfig/URL/search params and mapping error responses into KibaException) used by both streaming and non-streaming implementations.
| const response = await (request.timeoutSeconds ? timeoutPromise(request.timeoutSeconds, fetchOperation) : fetchOperation); | ||
| return response; |
There was a problem hiding this comment.
The timeout mechanism (timeoutPromise) likely rejects without aborting the underlying fetch/stream read, meaning network and parsing work can continue in the background after a timeout. Consider integrating AbortController so timeouts actively cancel the in-flight fetch and release resources for streaming requests.
Description