Skip to content
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

Closed
wants to merge 1 commit into from
Closed

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Jan 22, 2024

Resolves #5279

Well, i did not write a test yet, but I wanted to get from you some feedback first regarding the approach.

Checklist

@Uzlopak Uzlopak requested a review from a team January 22, 2024 04:50
@@ -24,6 +25,7 @@ const {
kOptions,
kRouteContext
} = require('./symbols.js')
const kWebStreamReadable = Symbol.for('nodejs.stream.readable')
Copy link
Member

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))
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

@Uzlopak Uzlopak Jan 22, 2024

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.

Copy link
Member

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
})

@climba03003 climba03003 mentioned this pull request Jan 25, 2024
4 tasks
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jan 25, 2024

Closing in favor of #5286

@Uzlopak Uzlopak closed this Jan 25, 2024
@Uzlopak Uzlopak deleted the handle-webstreams branch January 25, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return Response.body from a fetch call as a stream
3 participants