-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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: handle webstreams in reply.send #5280
Conversation
30d6076
to
0b96241
Compare
@@ -24,6 +25,7 @@ const { | |||
kOptions, | |||
kRouteContext | |||
} = require('./symbols.js') | |||
const kWebStreamReadable = Symbol.for('nodejs.stream.readable') |
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.
Should not use non-documented symbol.
We may do a similar check like typeof payload.getReader === 'function'
.
@@ -168,6 +170,11 @@ Reply.prototype.send = function (payload) { | |||
return this | |||
} | |||
|
|||
if (payload[kWebStreamReadable]) { | |||
onSendHook(this, Readable.fromWeb(payload)) |
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.
You should not change the payload here.
Readable.fromWeb
must provided inside onSendEnd
.
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.
...also, I think there are a few headers that should be preserved if it's a fetch Response
.
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.
It would be another PR, but identify Response
would be hard.
I don't know which function
would be specific enough to identify it.
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.
Why on onSendEnd?
What if in an onSend hook i check for a stream like it is done in @fastify/throttle? Imho must be done as early as possible.
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.
It looks like a partial support of WebStream
as we mutate the response in the middle.
I know that object
will be serialized and BufferLike
is already changed to Buffer
.
But, WebStream
should be special treated to allow better support or extendible like extends it to Response
in the middle.
Ultimately, WebStream
should be possible to do the same as NodeStream
or even Response
.
nodejs/node#42529
fastify.addHook('onSend', async (request, reply, payload) => {
if (payload instanceof ReadableStream) {
// we can use Response here
return new Response(payload, {
headers: { "Content-Type": "text/html" }
})
}
return payload
})
Closing in favor of #5286 |
Resolves #5279
Well, i did not write a test yet, but I wanted to get from you some feedback first regarding the approach.
Checklist
npm run test
andnpm run benchmark
and the Code of conduct