readable-stream icon indicating copy to clipboard operation
readable-stream copied to clipboard

`Readable.fromWeb` and `Readable.toWeb` do not seems to be properly implemented

Open Tpt opened this issue 2 years ago • 4 comments

First, thank you for your amazing work on readable-stream!

Readable.fromWeb an Readable.toWeb are calling the newStreamReadableFromReadableStream and newReadableStreamFromStreamReadable functions on the webStreamsAdapters.

However webStreamsAdapters is only defined as an empty object leading to a failure.

The same problem seems to affect Writable and Duplex.

Tpt avatar Aug 20 '22 07:08 Tpt

Thanks for reporting! Which Node.js version are you using? This module does not include WebStreams, so I won't expect those method to work... however we should likely throw a better exception.

mcollina avatar Aug 20 '22 08:08 mcollina

Thanks for reporting! Which Node.js version are you using?

NodeJS v18.7.0 so I would expect WebStreams to be available.

This module does not include WebStreams, so I won't expect those method to work... however we should likely throw a better exception.

What about having an implementation for cases like NodeJS 16+ and modern web browsers were WebStreams are available?

Tpt avatar Aug 20 '22 10:08 Tpt

@mcollina I think the ask is for these methods to work since the environment this package runs in (either Node to support several versions or browsers) often does have web streams?

benjamingr avatar Aug 20 '22 10:08 benjamingr

I understand now, thanks!

Would you like to send a PR? I'll be a bit low on time for the coming months.

mcollina avatar Aug 22 '22 06:08 mcollina

Hi @mcollina @benjamingr, since this is an old issue, just wanted to know if this is still relevant or if has it been solved in any way, if not I can work on this.

Had a couple of questions:

  1. What should the Readable.toWeb and Readable.fromWeb return if the webStreamAdapters is an empty object?
  2. What is the purpose of the newStreamReadableFromReadableStream and newReadableStreamFromStreamReadable functions, as I don't see them defined anywhere in the codebase?

RishabhKodes avatar Feb 24 '23 20:02 RishabhKodes

Those are node core functions: https://nodejs.org/api/stream.html#streamreadablefromwebreadablestream-options

mcollina avatar Feb 24 '23 23:02 mcollina

Those functions have not been extracted correctly and should have been removed from this module. Use them from require('stream').

mcollina avatar Feb 24 '23 23:02 mcollina

I think the ask is for these methods to work since the environment this package runs in (either Node to support several versions or browsers) often does have web streams?

@mcollina addressing the issue of the empty webStreamsAdapters object if the webstreamAdapters is undefined, we can solve it by using a condition and returning something else other than the empty object (as @benjamingr mentioned). That might solve this issue and the failures cause to the users using this method.

RishabhKodes avatar Feb 25 '23 21:02 RishabhKodes

Well, I think we should not have these methods to begin with and remove them in the build process.

mcollina avatar Feb 25 '23 21:02 mcollina

Would you suggest removing both methods from the readable.js, so as not to cause further confusion on the origins of these methods? or as you mentioned, modify the build (build.mjs) file and somehow remove it over there.

RishabhKodes avatar Feb 25 '23 22:02 RishabhKodes

Change build.mjs so that they are removed from the build when this module is extracted from Node.js.

mcollina avatar Feb 26 '23 08:02 mcollina

Docs need to be updated too

tysonrm avatar Jun 21 '24 13:06 tysonrm