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

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

Closed
Tpt opened this issue Aug 20, 2022 · 11 comments · Fixed by #506
Closed

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

Tpt opened this issue Aug 20, 2022 · 11 comments · Fixed by #506
Labels

Comments

@Tpt
Copy link

Tpt commented Aug 20, 2022

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
Copy link
Member

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
Copy link
Author

Tpt commented Aug 20, 2022

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
Copy link
Member

@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
Copy link
Member

I understand now, thanks!

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

@RishabhKodes
Copy link
Contributor

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
Copy link
Member

@mcollina
Copy link
Member

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

@mcollina mcollina added the bug label Feb 24, 2023
@RishabhKodes
Copy link
Contributor

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
Copy link
Member

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

@RishabhKodes
Copy link
Contributor

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants