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.
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
feat: separate strings out of primary SynthesizeStream pipe #957
feat: separate strings out of primary SynthesizeStream pipe #957
Changes from 1 commit
c2fc175
a3acd2d
e7aabca
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only one that makes me uneasy because the other existing error event has one arg, which is an Error object that's been formatted a little:
it is very likely that users already have handlers set up for the Error event expecting this type of object. I think it would be prudent here to either a) rename this event to something unique or b) try to put together a similarly formatted Error object to send back as the first arg. There are pros and cons to both - what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted my previous response, after re-reading the docs, it is not clear to me what the behavior on error from the socket, so I'd like to test some stuff first before giving a response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, so played around a bit more with this and it looks like for things like messing up the codec or forgetting
text
argument, the JSON object is sent over theonmessage
channel to the user and then with an additional message on theonclose
that says "see previous message for the error details". I'd probably lean towards making them emit on the same channel (and unify warnings to a similar emit format, though on a different channel), as the end result is all the same regardless of the exact reason.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good to me, thanks for looking into it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything else looks good. Once you update the error handler, I'll approve and merge 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, updated the error handler to look like the
onerror
signature. I did not touch thewarnings
one as I'm not sure there's a good way to mimic the warnings as JS does not really have a good concept of warnings like python unfortunately.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think that's fine and there's no existing API there so I'm not worried about it