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

Consider removing readable-stream dependency #150

Open
Fuzzyma opened this issue Jan 19, 2025 · 5 comments
Open

Consider removing readable-stream dependency #150

Fuzzyma opened this issue Jan 19, 2025 · 5 comments

Comments

@Fuzzyma
Copy link

Fuzzyma commented Jan 19, 2025

Looking at your CI runs, its seems like the lowest Node Version that is supported is Node 18.
Afaik, Node 18 has a native ReadableStream implementation so I was wondering while readable-stream is still used.

bl is downloaded about 150M times a month and the traffic created by the readable-stream dependency alone is over 8TB. Considering that and 2 less deps to care about, I think it would be a win to get rid of readable-stream

I saw that you bumped the version of readable-stream in your latest major so if there was already a discussion about it, please point me to it :).

I would be happy to create a PR to do the necessary changes!

@rvagg
Copy link
Owner

rvagg commented Jan 20, 2025

Node has always had readable-stream in it, it started life as an extract of the streams library and now it's the source of truth (or last I checked). The reason for it's inclusion here, originally at least, was to have a stable streams library to work against instead of relying on the variability of whatever is bundled in Node, and to work without any bundling issues within that various browser bundling formats. If you have information that changes any of those dynamics then I'd be happy to hear them (although with limited mental bandwidth to absorb them because the whole browser bundling ecosystem still makes me grumpy).

@Fuzzyma
Copy link
Author

Fuzzyma commented Jan 20, 2025

Oh I wasn't aware that this package is used in the browser as well. However, as far as I know node 16 offers the WebStream api and since it's a browser standard it should be fairly stable and "just work"

I will have to check the minimum supported browsers tho

@mcollina
Copy link
Collaborator

Here is the TL;DR from the person shipping readable-stream:

  1. the source of truth is node:stream
  2. the source code of readable-stream is a copy of what it is shipped as v18, hopefully we'll update to v24 later this year once v18 phase out of LTS. This is not a problem for this module, as it currently supports Node v18+.
  3. readable-stream is guaranteed to work in browsers, at least with some bundlers. node:stream is not polyfilled by bundlers anymore, therefore by dropping readable-stream browser support would break.

My recommendation is:

  • use readable-stream for long-term node compatibility (way past EOL, we still ship fixes for 1.x, 2.x, 3.x lines)
  • use readable-stream for browser compatibility

Consider what @Fuzzyma is asking, the solution here is relatively simple: split https://github.com/rvagg/bl/blob/master/bl.js (the browser thing) into its own module for browser usage, of which bl will depend upon. Then drop browser support for bl proper by switching to node:stream.

@43081j
Copy link

43081j commented Jan 20, 2025

This kind of thing itches my brain so just a few opinions here:

The fact that this package uses node:stream means it is fundamentally designed in a way that won't work on browsers, by choice.

If it was ever intended to work in a browser, it shouldn't use those libraries.

Polyfilling them is a workaround for that design choice.

A Node.js Buffer list collector, reader and streamer thingy.

Given the description of the project, too, it is clear to me that this is a node library.

If a consumer wants to use a node library in a browser, it should be on them to polyfill things rather than the maintainer of this library.

I don't think this project should've ever had a browser entry point.

I'd argue this is probably the case for almost all consumers of readable-stream (the npm package).

@talentlessguy
Copy link

talentlessguy commented Jan 20, 2025

my 2c

  1. for browser compat, Web Streams could be used, or for both node and web since Node.js supports web streams as well. (And the lowest supported version for bl would be v22)

  2. it's not a package's job to polyfill node built-ins, that's a bundling tool's job (usually it's just a setting or two to enable polyfilling)

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

5 participants