Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add optional data parameter for client requests #438

Merged
merged 5 commits into from
Jul 31, 2023

Conversation

pjlast
Copy link
Contributor

@pjlast pjlast commented Jul 28, 2023

Add an optional data?: any parameter that's returned as-is to clients. This allows us to add something like a messageID to completion requests and keep track of which request we're receiving back.

This is especially useful for streaming responses, where we'd like to be able to handle different streamed responses concurrently, and route them based on the message ID.

Also, instead of returning null when the message is done, it returns only the data field (if present). This way we know that that message is finished and we can handle it appropriately.

Test plan

🤞

@@ -131,7 +141,7 @@ export async function createClient({
isMessageInProgress = true
transcript.addInteraction(interaction)

sendTranscript()
sendTranscript(options?.data)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the point of this sendTranscript here that sends an empty message?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure, I did not know what it is... maybe we can remove? I can check some more later

Comment on lines -134 to -135
sendTranscript()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this sendTranscript that sends an empty message serves any purpose. Removing it, unless instructed otherwise

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I don't know, it seems to get some transcript from the closure but I ran your changes and didn't notice a difference 🙈

Comment on lines -134 to -135
sendTranscript()

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I don't know, it seems to get some transcript from the closure but I ran your changes and didn't notice a difference 🙈

@pjlast pjlast merged commit 222f929 into main Jul 31, 2023
8 checks passed
@pjlast pjlast deleted the pjlast/return-client-data-for-recipes branch July 31, 2023 12:58
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.

3 participants