Skip to content

Convert JS SDK streaming APIs from callbacks to async iterables#545

Open
Copilot wants to merge 5 commits intomainfrom
copilot/update-audio-client-inerrable-pattern
Open

Convert JS SDK streaming APIs from callbacks to async iterables#545
Copilot wants to merge 5 commits intomainfrom
copilot/update-audio-client-inerrable-pattern

Conversation

Copy link
Contributor

Copilot AI commented Mar 23, 2026

  • Convert JS SDK streaming APIs from callbacks to async iterables
  • Add return() hook to async iterators to prevent unbounded buffering on early break
  • Add guards in streaming callbacks to skip work after error or cancellation
  • Fix test assertions to assert synchronous throws directly
  • Replace O(n) chunks.shift() with O(1) head-index dequeue with compaction
  • Guard against concurrent next() calls with nextInFlight flag
  • Add comment explaining native stream cancellation limitation in return()
  • Fix docs example for completeStreamingChat(messages, tools) overload to pass tools
  • Regenerate TypeDoc API docs
  • Type-check, code review, and security scan
  • Add comments explaining why local variable captures are needed (closures lose this)
  • Add comments clarifying promise-resolve wake-up pattern in .then() handler
  • Add structural comments explaining the AsyncIterable/AsyncIterator factory pattern
  • Apply same readability improvements to chatClient.ts

⚡ Quickly spin up Copilot coding agent tasks from anywhere on your macOS or Windows machine with Raycast.

Copilot AI and others added 2 commits March 23, 2026 22:14
…pattern

Addresses #507 - ChatClient.completeStreamingChat() and
AudioClient.transcribeStreaming() now return AsyncIterable<any>
instead of accepting callback functions, enabling the standard
`for await (const chunk of ...)` pattern.

BREAKING CHANGE: completeStreamingChat() and transcribeStreaming()
no longer accept callback parameters. Use `for await` iteration.

Co-authored-by: baijumeswani <12852605+baijumeswani@users.noreply.github.com>
Agent-Logs-Url: https://github.com/microsoft/Foundry-Local/sessions/f8227df8-7016-4fe1-9884-56bbdcf326b7
@vercel
Copy link

vercel bot commented Mar 23, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
foundry-local Ready Ready Preview, Comment Mar 24, 2026 10:16pm

Request Review

@baijumeswani
Copy link
Collaborator

@copilot do you need to make similar changes to rust sdk as well?

Copy link
Contributor Author

Copilot AI commented Mar 23, 2026

@copilot do you need to make similar changes to rust sdk as well?

No — the Rust SDK already returns stream types (ChatCompletionStream and AudioTranscriptionStream via JsonStream<T>) from complete_streaming_chat and transcribe_streaming, so it already supports the idiomatic async iteration pattern. Only the JS SDK had the callback-based API.

Copilot AI requested a review from baijumeswani March 23, 2026 23:50
@baijumeswani baijumeswani marked this pull request as ready for review March 24, 2026 00:11
Copilot AI review requested due to automatic review settings March 24, 2026 00:11
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the JS SDK streaming APIs to follow the common for await (...) pattern by removing callback-based overloads and returning AsyncIterable streams for chat completions and audio transcription.

Changes:

  • Refactors ChatClient.completeStreamingChat() and AudioClient.transcribeStreaming() to return AsyncIterable<any> instead of accepting callbacks.
  • Updates streaming tests and examples to iterate with for await (...).
  • Regenerates/updates TypeDoc + READMEs to reflect the new streaming API surface.

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
sdk/js/src/openai/chatClient.ts Converts streaming chat completion from callback-based API to AsyncIterable adapter.
sdk/js/src/openai/audioClient.ts Converts streaming transcription from callback-based API to AsyncIterable adapter.
sdk/js/test/openai/chatClient.test.ts Migrates streaming chat tests to for await iteration and removes callback validation tests.
sdk/js/test/openai/audioClient.test.ts Migrates streaming audio tests to for await iteration and removes callback validation tests.
sdk/js/examples/chat-completion.ts Updates streaming example to for await usage.
sdk/js/examples/tool-calling.ts Updates streaming tool-calling example to for await usage.
sdk/js/examples/audio-transcription.ts Updates streaming transcription example to for await usage.
sdk/js/README.md Updates SDK README examples to use async iteration for streaming.
README.md Updates root README streaming transcription snippet to async iteration.
samples/js/native-chat-completions/app.js Updates sample streaming chat usage to async iteration.
samples/js/chat-and-audio-foundry-local/src/app.js Updates sample streaming summary generation to async iteration.
samples/js/audio-transcription-example/app.js Updates sample streaming transcription usage to async iteration.
sdk/js/docs/classes/ChatClient.md Updates generated API docs for new AsyncIterable streaming signature + example.
sdk/js/docs/classes/AudioClient.md Updates generated API docs for new AsyncIterable streaming signature + example.
sdk/js/docs/classes/ChatClientSettings.md Regenerated docs reflecting optional property markers.
sdk/js/docs/classes/AudioClientSettings.md Regenerated docs reflecting optional property markers.
sdk/js/docs/classes/ResponsesClientSettings.md Regenerated docs reflecting optional property markers.
sdk/js/docs/classes/Model.md Updates generated throws description text.
sdk/js/docs/README.md Regenerated docs reflecting optional property markers and signatures.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@baijumeswani
Copy link
Collaborator

@copilot please address this feedback.

…dation tests

- Add return() method to async iterators in chatClient.ts and audioClient.ts
  to prevent unbounded buffering when consumers break out of for-await early
- Add cancelled/error guards in native streaming callbacks to skip work
  after error or cancellation
- Fix validation tests to assert synchronous throws directly instead of
  wrapping in unreachable for-await loops

Co-authored-by: baijumeswani <12852605+baijumeswani@users.noreply.github.com>
Agent-Logs-Url: https://github.com/microsoft/Foundry-Local/sessions/69aaf402-f4bd-496b-9d37-e139a3223cdb
Copy link
Contributor Author

Copilot AI commented Mar 24, 2026

@copilot please address this feedback.

Addressed all 6 items from the review in commit 669ad5e:

  1. return() hook added to both chatClient.ts and audioClient.ts async iterators — sets a cancelled flag, clears the buffer, and wakes any pending next() so early break from for await stops accumulating chunks.
  2. Callback guards — the native streaming callback now checks if (cancelled || error) return; at the top, skipping JSON parse and push work once iteration is done or errored.
  3. Test fixes — validation tests in chatClient.test.ts and audioClient.test.ts now assert the synchronous throw directly on the method call, removing the unreachable for await loop.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 7 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@baijumeswani
Copy link
Collaborator

@copilot please address this feedback.

…tion docs, tools example

- Replace O(n) chunks.shift() with head-index + compaction for O(1) dequeue
- Add nextInFlight guard to throw on concurrent next() calls
- Add comment in return() explaining native stream cancellation limitation
- Update completeStreamingChat JSDoc example to show both overloads
- Regenerate TypeDoc API docs

Co-authored-by: baijumeswani <12852605+baijumeswani@users.noreply.github.com>
Agent-Logs-Url: https://github.com/microsoft/Foundry-Local/sessions/dfa24137-7f37-4aa6-adb9-a735b7ce4878
Copy link
Contributor Author

Copilot AI commented Mar 24, 2026

@copilot please address this feedback.

All 7 items addressed in 1d4f8d7:

  1. O(1) dequeue — replaced chunks.shift() with a head-index approach in both chatClient.ts and audioClient.ts; the array compacts when all buffered chunks are consumed (head === chunks.length).
  2. Concurrent next() guard — added a nextInFlight flag that throws if next() is called before the previous call resolves.
  3. Native cancellation docsreturn() now documents that CoreInterop.executeCommandStreaming has no abort support, so the koffi callback may still fire but will no-op via the cancelled guard.
  4. Docs example fix — the completeStreamingChat JSDoc now shows both the messages-only and messages+tools overloads; TypeDoc regenerated.

// Don't throw from callback - store first error and stop processing
error = new Error(`Failed to parse streaming chunk: ${e instanceof Error ? e.message : String(e)}`, { cause: e });
return;
const coreInterop = this.coreInterop;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this necessary? can we just use this.coreInterop on L#130?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the local capture is necessary. The [Symbol.asyncIterator]() method on line 123 is a regular method on the returned object literal, so this inside it refers to that object — not the AudioClient instance. Added a comment explaining this in d39bad8.

error = new Error(`Failed to parse streaming chunk: ${e instanceof Error ? e.message : String(e)}`, { cause: e });
return;
const coreInterop = this.coreInterop;
const modelId = this.modelId;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question as for coreInterop

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reason as coreInteropthis is unavailable inside the [Symbol.asyncIterator]() closure. Comment added in d39bad8.

if (resolve) {
const r = resolve;
resolve = null;
r();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this do? can we include a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It resolves the promise that a pending next() call is awaiting, waking it up so it can see that the stream has ended (done = true). Added clarifying comments in d39bad8.

}
});

return {
Copy link
Contributor

@prathikr prathikr Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a nested return statement? kind of unintuitive. please add a comment why this is designed this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the standard AsyncIterable/AsyncIterator two-level factory pattern: [Symbol.asyncIterator]() returns the iterator object (with next() and return() methods) that for await consumes. Added a structural comment explaining this in d39bad8.

const tools = Array.isArray(toolsOrCallback) ? toolsOrCallback : undefined;
const callback = (Array.isArray(toolsOrCallback) ? maybeCallback : toolsOrCallback) as ((chunk: any) => void) | undefined;

public completeStreamingChat(messages: any[]): AsyncIterable<any>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same general comments as in audioClient

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied the same comments to chatClient.ts in d39bad8.

@baijumeswani
Copy link
Collaborator

@copilot please address the following feedback from @prathikr's pr comments.

…tern, and iterator structure

Address @prathikr review feedback:
- Explain why coreInterop/modelId are captured to local variables (this is
  not available inside [Symbol.asyncIterator]() on the returned object)
- Explain the AsyncIterable/AsyncIterator factory pattern with structural comments
- Add comments on .then() handler explaining the promise-resolve wake-up pattern
- Apply same comments to both audioClient.ts and chatClient.ts

Co-authored-by: baijumeswani <12852605+baijumeswani@users.noreply.github.com>
Agent-Logs-Url: https://github.com/microsoft/Foundry-Local/sessions/b62befe1-e407-4668-82a1-a2a22267eed9
Copy link
Contributor Author

Copilot AI commented Mar 24, 2026

@copilot please address the following feedback from @prathikr's pr comments.

Addressed all 5 comments from @prathikr in d39bad8:

  1. Local variable captures (lines 113-114) — added comment explaining this is not available inside [Symbol.asyncIterator]() since it's a regular method on the returned object literal.
  2. r() call (line 160) — added comment explaining it resolves the pending next() promise to wake it up when the stream ends.
  3. Nested return (line 178) — added structural comment explaining the AsyncIterable/AsyncIterator two-level factory pattern.
  4. chatClient.ts — same comments applied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants