nodejs / readable-stream

Node-core streams for userland
https://nodejs.org/api/stream.html
Other
1.03k stars 227 forks source link

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

Closed Tpt closed 1 year ago

Tpt commented 2 years ago

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.

mcollina commented 2 years ago

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.

Tpt commented 2 years ago

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?

benjamingr commented 2 years ago

@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 commented 2 years ago

I understand now, thanks!

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

RishabhKodes commented 1 year ago

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?
mcollina commented 1 year ago

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

mcollina commented 1 year ago

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

RishabhKodes commented 1 year ago

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.

mcollina commented 1 year ago

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

RishabhKodes commented 1 year ago

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.

mcollina commented 1 year ago

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

tysonrm commented 4 months ago

Docs need to be updated too