Adhere to the full Streams API

[mcollina]

Currently send does not implement the Streams API (even if it inherits from Stream), and in fact inside send(..).pipe(res), it does stream.pipe(res).

Specifically, we had to use an "hack" to make it work well on Fastify. See fastify/fastify-static#46, where we had to pipe it through a modified PassThrough.

I'm thinking of changing the API to something:

const { stream, headers, statusCode } = await send(req, path, options)

As well as:

send(req, path, options, (err,  { stream, headers, statusCode }) => {
  // ...
})

I would like to also remove the dependency on on-finished and some of the others modules that were needed in the 0.8/0.10 days.

I think this work should help as well with http2 compatibility as well. I'm happy to prototype this work in a forked module as well, just let me know.

cc @apapirovski

[dougwilson]

Yea, definitely would like to see. Node.js 0.8 will be dropped with the next major but 0.10 support won't be removed anytime soon, so if it's possible to still support 0.10 that would be super awesome.

[dougwilson]

FWIW I feel like the current API is pretty janky so totally open to an API that flows better, especially the output side. Many would like to see this write to a transform stream to do transformations, which sounds like you're ask too.

[dougwilson]

In addition for Node.js versions, everything in this org tries to support as many Node.js versions as possible -- it's part of our philosophy. We have the ideal support line (down to 0.10 currently) and the hard line of must go down to at least (4.0 currently). Really the minimum version is negotiable between those two, and if 0.10 isn't picked there should be some good reasons to why it's impossible to support lower (in the past we have used compat helps like on-finished to accomplish tasks like this, so perhaps it's just to make a new one).

[mcollina]

Yes, I need to apply a transform afterwards (compression).

I'll see what I can come up with, I'll target Node 4, and then we can see if we can stretch it to 0.10.

I'm in your same shoes for backward compat, as readable-stream@2 (pulled from Node 8) supports down to Node 0.8.

[bjohansebas]

@pillarjs/express-tc Is this something we still want?

[mcollina]

I would actually recommend to reconcile with https://github.com/fastify/send, which already has a variation of this implemented.

[bjohansebas]

I don't fully understand what you mean by reconciling it in this case. Could you elaborate, please?

[mcollina]

Basically take @fastify/send as the new base of this module and reconcile the two forks.

[bjohansebas]

That sounds good to me, although it was required for the TC to agree. I'll add it to the agenda.

edit: Okay, the label is not there. Once it is added, I will add it to the agenda.

[wesleytodd]

We are pretty sure this will need to be a part of the express v6 roadmap, and we have some STF funding which also partly covers this work. Timeline on this is likely to be later this year, just to make sure we are clear.

@mcollina If you were willing to, we would love to find a time to sync up with you on how to move forward here. So would you be willing to run us all through some of what your goals are here, what specific technical goals we should be aware of, and lastly what we will need to be aware of when reconciling these to test the ecosystem of modules that depend on this.

[blakeembrey]

@mcollina is there specific attribution you’d want when doing this work? I’d like to prioritize this and understood the comment as mostly doing a PR based on your branch of work into send but I haven’t begun digging in just yet. Some questions:

• Set whoever maintains it as author or co-author of the PR?
• Would they want to stay on to continue maintenance of the project after it merges?
• Or would you want us to start from your repo and work the opposite direction, PR to the fork? I’m thinking this will be trickier but want to confirm intentions of using it as the new base

[wesleytodd]

I think it is more than just that. Similar to the other issues in question, I think we should be working toward an api which we could upstream to node core someday, which likely would enable things like sending the file directly to the socket without ever hitting JS (a thing discussed in the web server frameworks team IIRC). Unless someone has a plan for that in the existing modules, I think we should get a lay of the land here before making any changes. Hence why we discussed getting @mcollina (and team if they can) to sync on this before making any concrete moves.

[wesleytodd]

I guess I should clarify, that is unless you can work out a way to do this in a backwards compatible way. If so, that would be awesome and then we could try to land it in v5.

[mcollina]

is there specific attribution you’d want when doing this work?

I recommend opening a matching issue on the other repo to make the rest of Fastify team know where this is at.

Set whoever maintains it as author or co-author of the PR?

The @fastify/send codebase diverged quite a bit since then. I would say that most of the work was done in fastify/send#77 by @climba03003, plus a lot of optimization work by @Uzlopak and maintenance work by @Fdawgs and myself.

Would they want to stay on to continue maintenance of the project after it merges?

We have been doing the work atm due to the lack of activity on this one. We have aggressively dropped support for old Node.js runtimes (one of the things that made maintaining this very problematic). Less maintenance work is good for everybody if it's possible.

@fastify/send is as close to a rewrite as possible.

Or would you want us to start from your repo and work the opposite direction, PR to the fork? I’m thinking this will be trickier but want to confirm intentions of using it as the new base

Open an issue on @fastify/send and check with the rest of the community.

---

Similar to the other issues in question, I think we should be working toward an api which we could upstream to node core someday, which likely would enable things like sending the file directly to the socket without ever hitting JS

This is not that API unfortunately, as it still rely on Node.js streams.

I guess I should clarify, that is unless you can work out a way to do this in a backwards compatible way. If so, that would be awesome and then we could try to land it in v5.

@fastify/send is 100% not backwards compatible.

[wesleytodd]

This is not that API unfortunately, as it still rely on Node.js streams.

Yep, I was mainly saying I am more interested in working toward that future api since this is not going to ship to the majority of it's consumers for at least a year. And then because we will be supporting express v6 for something like 1-2 years (TBD) after that it will be much better if we can skip a step. If this work had been done when this issue was opened originally I would have a completely opposite opinion, but I think at this point short term thinking is low value.

TLDR: we have to land a breaking change no matter what, why not choose the better of the two options?

[mcollina]

I kind of concur with the assessment.