feat: separate strings out of primary SynthesizeStream pipe #957
Conversation
Codecov Report
@@ Coverage Diff @@
## master #957 +/- ##
=======================================
Coverage 64.28% 64.28%
=======================================
Files 2 2
Lines 14 14
Branches 3 3
=======================================
Hits 9 9
Misses 4 4
Partials 1 1Continue to review full report at Codecov.
|
lib/synthesize-stream.ts
Outdated
| self.emit('words', message, chunk); | ||
| } | ||
| else if (json['error']) { | ||
| self.emit('error', message, chunk); |
There was a problem hiding this comment.
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:
socket.onerror = event => {
const err = new Error('WebSocket connection error');
err.name = SynthesizeStream.WEBSOCKET_CONNECTION_ERROR;
err['event'] = event;
self.emit('error', err);
self.push(null);
};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.
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.
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 the onmessage channel to the user and then with an additional message on the onclose 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.
That sounds good to me, thanks for looking into it
There was a problem hiding this comment.
Everything else looks good. Once you update the error handler, I'll approve and merge 👍
There was a problem hiding this comment.
Alright, updated the error handler to look like the onerror signature. I did not touch the warnings 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.
Yeah I think that's fine and there's no existing API there so I'm not worried about it
|
🎉 This PR is included in version 4.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Fixes #956
As discussed in the issue, the SythesizeStream is currently outputting all information, be it strings or binary data to the pipe (after converting to a Buffer). However, following the example, this will lead to a file that is invalid as it will append both the text and binary to the same file.
This PR separates out the information, and outputs the information onto their own relevant channel as documented for the different types of information strings on the documentation:
(the bottom of the second demonstrates also that the strings may come intermixed with the binary audio information)
The various things documented as being possible emitted:
While this is a breaking change, as it's breaking things to be more expected and inline with how the API is thought to work, putting it as a minor release instead of a major release, which also does not require any documentation change.
No idea how to include a test for this change as well since it looks like the existing test does not directly test the data piped out of the socket.
Checklist
npm testpasses (tip:npm run autofixcan correct most style issues)Note: Labeled this a
featinstead offixso that it gets a minor release instead of a bugfix by semantic-release, which I think is probably more appropriate, but can rename the commit if desired.