Use ReadableStream#202
Conversation
zeke
left a comment
There was a problem hiding this comment.
I don't know much about the internal of this stuff, but this look sensible to me, and the tests are passing. I'm cool with shipping this, barring any opposition from @mattt
@MaxLeiter your PR body explains the purpose of this change, but can you add a bit more detail that I can use for some release notes about this change?
mattt
left a comment
There was a problem hiding this comment.
Thanks so much for taking a look at this, @MaxLeiter. Overall, this is looking much nicer! I have one refactoring suggestion to move parseEvent to a static method on the SSE class (patch here: 0001.patch)
The only blocker is that I think we need to define the async iterator for the class if we're keeping the existing usage in replicate.stream. Left a suggestion in a comment with some untested code.
Speaking of untested, I don't believe the stream method has any test coverage at the moment, because I couldn't figure out how to get SSE to play nicely with nock. I tested this locally by hand for the initial version, but it'd be great to automate this.
Co-authored-by: Mattt <mattt@me.com>
|
Hi @MaxLeiter. Thanks again for your help with this. We just merged #214 and released v0.28.0, which should resolve any issues you were having. We also added a Cloudflare Workers integration test suite with #217. If you have a chance, please give that a try and confirm that's working for you 🙇 |
Fixes support for Cloudflare workers and vercel edge runtime. ReadableStreams are available in Node.js since Node 18: https://nodejs.org/docs/latest-v18.x/api/webstreams.html#web-streams-api