Refactor Stream to use eventstream-parser library #214
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aron
force-pushed
the
refactor-stream
branch
3 times, most recently
from
7e68022 to
18a182d
Compare
aron
changed the title
aron
force-pushed
the
refactor-stream
branch
3 times, most recently
from
0515a03 to
63ec753
Compare
|
@mattt I've now bundled the |
|
@mattt I'm pretty confident this one is in good shape. I'll merge it today unless you have any further concerns. |
mattt
approved these changes
Mar 12, 2024
Closed
aron
added a commit
that referenced
this pull request
Mar 12, 2024
This fixes a regression introduced with #214 where we were not exiting correctly when getting the `"done"` event from the server. This was picked up by the introduction of the CloudFlare integration tests added in #217 which uses the streaming API. Once the fix was added it turns out that the `nock()` tests were incorrectly passing due to some internal weirdness when using `respondWith` and a `Readable` object. I wasn't able to get this working without hitting a different error: TypeError: Invalid state: Controller is already closed It looks like nock is retaining some global state somewhere in it's implementation and streams are being retained across requests. No combination of resetting mocks seemed to fix it. In the end I just mocked out the fetch function passed into the `createReadableStream` library and returned a `Response`. I think we should probably do this everywhere rather than use `nock()` as the Request/Response APIs provided by fetch are much better now than the old node http lib.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Based on top of the unit tests added in #211 and taking inspiration from #202 this PR aims to simplify our EventSource parsing logic to use a standardized library,
eventsource-parser, designed to work across different environments including browsers, Node and Deno.This is bundled into the repository under the vendor/eventsource-parser/stream.js file, bundlejs was used to create the file and there is documentation in the README about how to regenerate it.
The stream is now built on top of the
ReadableStreamprimitive. Which is available in Node >= 18.I've also removed the
fetchshim from the tests as it's outdated and doesn't support streaming thebodyproperty. This just required updatingnockto use a v14 beta which supportsfetch.I've tested it on Cloudflare workers, but unfortunately we've added a bunch more node dependencies that need to be fixed before it'll work there. See #215 for more details on what needs to be done next.