-
Notifications
You must be signed in to change notification settings - Fork 218
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: web responses with streaming support #436
Conversation
Codecov Report
@@ Coverage Diff @@
## main #436 +/- ##
==========================================
- Coverage 77.26% 75.52% -1.74%
==========================================
Files 26 26
Lines 2617 2644 +27
Branches 376 381 +5
==========================================
- Hits 2022 1997 -25
- Misses 595 647 +52
|
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.
Thank you for adding this β€οΈ
I have a few questions :
What happens with the response 2nd argument ?
setResponseStatus(event, 208, "hello-status");
return new Response("valid", {
status: 207,
statusText: "bye-status" })
Does it works with Transform Streams ?
const response = new Response(`Hello world !`);
const { readable, writable } = new TransformStream();
response.body?.pipeTo(writable);
return new Response(readable, response);
It would be useful to have tests for theses.
Status code from
Readable streams should be normally chainable. I think we can improve tests for |
"/", | ||
eventHandler( | ||
() => | ||
new Response("Hello World!", { |
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.
please document this feature demo in readme.
π Linked issue
#73, #401
β Type of change
π Description
This PR adds support for directly returning a web Response object from event handlers, leveraging new streaming support for body! (#432)
It is also possible to explicitly use the newly exposed
sendWebResponse
utility.Note: The rest is pretty much simple and implementation is already used for
event.respondWith
(by @danielroe β€οΈ). I am thinking in next steps, reuse same utility forrespondWith
as well and deprecating pollyfills we initially made due to runtime incompatibilities. (#119)Also thanks to @Hebilicious for initial efforts in #395 to push for supporting native Responses β€οΈ
π Checklist