Refactor to remove the need for a PassThrough stream

[mcollina]

in fastify-static, we need a PassThrough object to convert Send to an actual Writable:

https://github.com/fastify/fastify-static/blob/master/index.js#L113-L140

I think this module should be refactored to in combination with fastify-static to remove the need for the wrapper completely.

[Uzlopak]

I have no clue what is needed to do it.

[mcollina]

Currently Send() is a stream-like object that don't really quack like a Stream.

It's also a "fake" synchronous API. There are two sets of asynchronous ops:

1. gathet the stats of the file
2. start the stream

Both of then happen after a call to pipe().

Essentially we need to refactor it so we split them. Get the metadata and then read the stream only if it's needed (in case or conditional gets).

[climba03003]

@mcollina Just some question before start.

Do you means Duplex instead of Writable? It should be a middle stream between fs.createReadStream and OutgoingMessage.

I assume it is what we would like to achieve?

import http from 'http'
import SendStream from '@fastify/send'
import { pipeline } from 'stream'

const app = http.createServer(function (req, res) {
  // the options include the duplex options
  const stream = new SendStream(req, req.url, { root: './' })
  pipeline(stream, res, function(err) {
    console.log(err)
  })
})

Does it means that we need to inspect on pipe and unpipe event?

graph TD
    A[SendStream] --> |created| B{Event Emitter}
    B --> |ready| C[Waiting for Pipe]
    B --> |pipe| D[Waiting for Ready]
    B --> |unpipe| E{Check Status}
    B --> |error| G[Cleanup and Destory]
    C --> |is pipe| F[Create File Readable]
    D --> |is ready| F
    E --> |flowing| G[Cleanup and Destory]
    E --> |finished| G
    E --> |never flow| C
    F --> |.pipe this| H[Streaming]
    H --> |end| G

Loading

[mcollina]

No, not really.

I think we need something like:

import http from 'http'
import send from '@fastify/send'
import { pipeline } from 'stream'

const app = http.createServer(async function (req, res) {
  // the options include the duplex options
  const { statusCode, headers, stream } = await send(req, req.url, { root: './' })
  res.writeHead(statusCode, headers)
  if (stream) {
    pipeline(stream, res, function(err) {
      console.log(err)
    })
  } else {
    res.end()
  }
})

It might be callback-based instead of promise-based.

[serzero2007]

No, not really.

I think we need something like:

import http from 'http'
import send from '@fastify/send'
import { pipeline } from 'stream'

const app = http.createServer(async function (req, res) {
  // the options include the duplex options
  const { statusCode, headers, stream } = await send(req, req.url, { root: './' })
  res.writeHead(statusCode, headers)
  if (stream) {
    pipeline(stream, res, function(err) {
      console.log(err)
    })
  } else {
    res.end()
  }
})

It might be callback-based instead of promise-based.

Please check if you are interested in my proof of concept implementation of proposed API here:
https://gist.github.com/serzero2007/c6c48b4c90c4318edcc17b458cf876bd

All imported functions are from this repository. The code is just a draft rewrite of existing code to use new API and not tested at all. Index support is not done yet and also it's not clear how to re-implement events in the new API.

Looking forward to your feedback.

[mcollina]

This is amazing! Would you mind to send a PR?

[serzero2007]

Well, it will take some time to make PR. It took 2 hours to get POC, but PR will take days.

There are questions we need to discuss first:

1. The API is changed dramatically. I think it's better to create a separate package with a new API, rather than breaking compatibility with previous API or bundling both APIs in the same package. What do you think?
2. Events API. We need somehow bring the functionality of it to a new API. Any suggestions?

[Uzlopak]

We use fastify/send for our fastify/static module. And only we are currently using this package. So you are imho free to break the api, as long it works flawless in fastify/static. We would also need to bench the performance. I dont know ad hoc, if we need the events api. Again, it has to work flawless in fastify/static.

[serzero2007]

Well, it obviously will require changes on the side of fastify/static, because API will have changed. For beginning, I will just drop event api. I suppose it is not a big deal to add it afterwards.

[mcollina]

This will give significant performance improvement to fastify-static, we are definitely happy to make the relevant the changes in that module.

We are not using the events API, so no problems there. We do support the index capability, however I think we could move it to @fastify/static or even drop that altogether.

[serzero2007]

You may track progress here:
https://github.com/serzero2007/send/blob/proofofconceptnewapi/lib/parsePath.js
https://github.com/serzero2007/send/blob/proofofconceptnewapi/lib/parseOptions.js
https://github.com/serzero2007/send/blob/proofofconceptnewapi/lib/send.js
https://github.com/serzero2007/send/blob/proofofconceptnewapi/lib/getStatAndPath.js
https://github.com/serzero2007/send/blob/proofofconceptnewapi/lib/conditionalGet.js
(I will edit this comment to include more files as they appear)

I am not experienced with writing tests in tap, so probably I will need some help here. For example, here:
https://github.com/serzero2007/send/blob/proofofconceptnewapi/test/send.test.js

[serzero2007]

I have a question. Why here

const etagL = etag.length
const isMatching = parseTokenList(ifNoneMatch, function (match) {
      const mL = match.length

      if (
        (etagL === mL && match === etag) ||
            (etagL > mL && 'W/' + match === etag)
      ) {
        return true
      }
})

do we compare the lengths of strings at all? Could we just compare strings itself? Is it a performance (micro)optimisation? Is it worth it after all? I am going to drop it and just compare strings:

function isIfNoneMatchFailure (req, context) {
    const ifNoneMatch = req.headers['if-none-match']

    if (ifNoneMatch === '*') return true
    if (typeof context.etag !== 'string') return false
    
    function checker(match) {
        if (match === context.etag) return true
        if ('W/' + match === context.etag) return true
        return undefined
    }

    const isMatching = parseTokenList(ifNoneMatch, checker, false)
    return isMatching
}