node-fetch/fetch-blob

maybe use `stream.Readable.toWeb`

jimmywarting opened this issue · 2 comments

stream.Readable.toWeb(streamReadable) got introduced in 17.0.0

while it's not necessary to use it... i would like to know if it brings any benefit from using it over fs.createWriteStream(path)
and instead do stream.Readable.toWeb(fs.createWriteStream(path))

I was just looking at this part of your code, and was considering this question. I came to the conclusion there would be no benefit. You create a Node stream with createWriteStream but then immediately async iterate over it (using yield *). Adding the stream.Readable.toWeb wrapper there would just be overhead that got immediately discarded.

A more aggressive change might be to change BlobDataItem.prototype.stream from being an async generator to returning a web stream. But since BlobDataItem is an internal class, I don't see what the benefit in doing that would be, either.

yup, it will just be unnecessary overhead.
the better solution would be to wait for nodejs/node#40606 to be fixed so it would be possible to go direct from file to whatwg stream, the extra overhead of converting it to buffer/node:stream/iterator/uint8array is not worth it...

the best solution would be to get rid of the toIterator entirely so that it never touches the main thread so that we could do something like this instead:

function concatenateReadables(readables) {
  const ts = new TransformStream();
  let promise = Promise.resolve();

  for (const readable of readables) {
    promise = promise.then(
     () => readable.pipeTo(ts.writable, { preventClose: true }),
     reason => {
       return Promise.all([
         ts.writable.abort(reason),
         readable.cancel(reason)
       ]);
     }
   );
  }

  promise.then(() => ts.writable.close(),
               reason => ts.writable.abort(reason))
         .catch(() => {});

  return ts.readable;
}

ref: https://streams.spec.whatwg.org/#example-identity-transform-usages