fix(js-client): stabilize hook headers, handle rejections, tolerate [DONE] streams#31
Open
cabljac wants to merge 1 commit into
Open
fix(js-client): stabilize hook headers, handle rejections, tolerate [DONE] streams#31cabljac wants to merge 1 commit into
cabljac wants to merge 1 commit into
Conversation
cabljac
force-pushed
the
fix/js-client-streaming-and-header-stability
branch
2 times, most recently
from
2c9256c to
e7e47b9
Compare
…DONE] streams Three fixes to the client SDK and React hooks: - useStream/useAction: depend on a content-derived headers key instead of the raw HeadersInit. Callers almost always pass an inline object literal (new reference every render), which re-created the stream executor and aborted any in-flight request on every render. - useStream/useAction: attach an internal no-op catch to the promise returned from execute(), so a fire-and-forget caller that ignores the promise does not trigger an unhandled rejection. Errors remain observable via `error` state, and awaiting callers still see the rejection. - parseStreamResponse: tolerate streaming-only flows that close gracefully after a [DONE] sentinel without a result envelope, resolving to undefined rather than throwing. A truncated stream (no result, no terminator) still throws, now with a clearer message. Adds regression tests for each.
cabljac
force-pushed
the
fix/js-client-streaming-and-header-stability
branch
from
e7e47b9 to
00c9221
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three correctness fixes on top of the
js-clientbranch, from a review of the client SDK + React hooks.1. 🔴 Inline headers silently abort streams (blocker)
useStream/useActionlisted the rawHeadersInitin their dependency arrays. Callers almost always pass an inline object literal — a new reference every render — so the stream executor was re-created and any in-flight request aborted on every re-render. Now they depend on a content-derivedserializeHeaders(...)key (latest value read via a ref), so equal headers keep the executor/executereferentially stable.2. 🟠 Unhandled rejection on fire-and-forget
execute()execute()re-throws after storing the error inerrorstate. A caller that doesn't await (e.g.onClick={() => execute()}) got an unhandled promise rejection. We now attach an internal no-op.catchto the returned promise; errors remain observable viaerrorstate and awaiting callers still see the rejection.3. 🟠 Stream end falsely errors without a
{result}envelopeparseStreamResponsethrew"Stream did not terminate correctly"whenever a stream closed without a{result}envelope. Streaming-only flows that close gracefully after a[DONE]sentinel now resolve toundefined. A genuinely truncated stream (no result, no terminator) still throws, with a clearer message.Tests
parse-streamgraceful[DONE], truncated-stream error) — 28 passVerified locally (client unit + type tests, react tests run against client source). Note: a separate pre-existing ESM build issue in
@genkit-ai/client(index.mjsimporting./aliases.js, cf. genkit-ai#5405) blocks running the react suite against the built lib — out of scope for this PR but worth a follow-up.