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

maybe use stream.Readable.toWeb #126

Closed
jimmywarting opened this issue Oct 28, 2021 · 2 comments
Closed

maybe use stream.Readable.toWeb #126

jimmywarting opened this issue Oct 28, 2021 · 2 comments

Comments

@jimmywarting
Copy link
Contributor

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

@dubiousjim
Copy link

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.

@jimmywarting
Copy link
Contributor Author

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

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

No branches or pull requests

2 participants