fix(rest): support ReadableStream as response body #974
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 11152dc:
|
Aprillion
force-pushed
the
581-readable-stream
branch
from
2ef433d
to
44834f2
Compare
|
|
| */ | ||
|
|
||
| test('Node does not support ReadableStream yet', () => { | ||
| expect(typeof ReadableStream).toBe('undefined') |
There was a problem hiding this comment.
This test needs to be rewritten, it doesn't test anything as of yet.
There was a problem hiding this comment.
the idea was to fail the test if Node adds ReadableStream ¯\_(ツ)_/¯ if you have something in mind that can be actually tested, that would be awesome!
| rest.get('/sse', (req, res, ctx) => { | ||
| return res( | ||
| ctx.status(200), | ||
| ctx.set('Content-Type', 'text/plain'), |
There was a problem hiding this comment.
I believe that such a response must also specify that its body is a stream.
There was a problem hiding this comment.
apparently not necessary for a stream of text/plain, at least in the integration test setup..
| const res = await request('/sse') | ||
| const status = res.status() | ||
| const headers = await res.allHeaders() | ||
| const text = await res.text() |
There was a problem hiding this comment.
We need to ensure that res.text() actually verifies that you can read that response stream once more on the usage surface (assert that we're correctly piping the original response stream into PassThrough in the worker).
Aprillion
force-pushed
the
581-readable-stream
branch
from
44834f2
to
db0521b
Compare
Aprillion
force-pushed
the
581-readable-stream
branch
from
db0521b
to
11152dc
Compare
kettanaito
force-pushed
the
main
branch
3 times, most recently
from
548fbd0
to
fed3b21
Compare
|
I'm going to close this in favor of #1288. This proposed change has served as an excellent example of how to reconstruct a readable stream within the worker. I've gone with a slightly different approach but this was tremendously helpful. |
Released: v0.43.0 🎉This has been released in v0.43.0! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
Fixes #581 .. but in a totally hacky* way and only tested with a stream of string chunks.
If improvements are needed before merge, please feel free to edit the PR or close it and just take inspiration for some better solution 😅 But I'm afraid I won't have more time until next weekend.
*hacky => original code counts that there will be only 1 message from client, and that will resolve a Promise... but when multiple messages come back, only one of them can resolve a promise - but even after the Promise is resolved, the onmessage handler lives inside the closure of a new Promise function, so it's a bit unexpected that we can handle multiple messages there even AFTER the Promise is resolved