nodejs / readable-stream

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

readable-stream v4 #439

Closed mcollina closed 2 years ago

mcollina commented 4 years ago

I think it's time to start planning readable-stream v4. Which version of Node.js should we pull from?

I would be ambitious and target v14. I would also drop support for Internet Explorer, as it will go EOL later this year.

cc @ronag @vweevers @mafintosh

mcollina commented 4 years ago

We also have some other things we should fix:

  1. rollup support
  2. webpack 5 support #435
ronag commented 4 years ago

I would be ambitious and target v14.

+1

I would also drop support for Internet Explorer, as it will go EOL later this year.

+1

mafintosh commented 4 years ago

SGTM

ljharb commented 4 years ago

@mcollina i still maintain that it's bad for the ecosystem to try to "fix" what webpack 5 broke, and would prefer to ask webpack 5 users to enable the workaround.

vweevers commented 4 years ago

Thinking further ahead, could stream ultimately use queueMicrotask in both node and browsers? After Node.js 10 is EOL (in 2021). Then stream is guaranteed to behave the same in node and browsers.

ronag commented 4 years ago

Thinking further ahead, could stream ultimately use queueMicrotask in both node and browsers?

Isn't that a case of just using Promise.resolve().then(cb) instead of process.nextTick(cb) (I think we are dropping IE?). There would be performance implications. Not sure whether those would be noticeable or not. Doing such a change would be semver-major. I'm unsure of the consequences since Promise has higher priority than nextTick.

EDIT: Sorry, I was not aware Node had queueMicrotask. Though, the above performance and timing concerns might still apply.

vweevers commented 4 years ago

There is a timing concern, yes, because the queueMicrotask queue is processed after the nextTick queue. That could pose a challenge in node core tests and specific stream implementations (especially the older ones that implement their own .destroy() etc).

ronag commented 4 years ago

While I'm not against the idea. I'm concerned that queueMicrotask might be a bit overly ambitious right now and could stall.

mcollina commented 4 years ago

I would not push that change to the ecosystem. This can break in oh-so-many unexpected ways.

I tend to agree with @ljharb. However it would likely be on us to develop some strategy to support webpack 5.

tpluscode commented 4 years ago

ES modules, please 🙏

mcollina commented 4 years ago

what about es modules?

tpluscode commented 4 years ago

what about es modules?

Replace commonjs with modules.

It would be very nice if the package did not rely on built-ins and commonjs so that it's runs natively in browser

ljharb commented 4 years ago

You don’t have to “replace” anything to do that, and you still need a build tool for the browser anyways - you can ship both ESM and CJS.

tpluscode commented 4 years ago

I don't think I fully understand your point @ljharb

and you still need a build tool for the browser anyways

The build is only the last step when you ship a complete app. Buildless dev environment is all the rage now with tools like es-dev-server or vite

To set a tangible goal, I would like this to work in a modern browser:

<script type="module">
// could be a bare import if import maps are used
import { Readable } from 'http://jspm.dev/readable-stream'
</script>

This will be impossible as long as anything in the dependency tree of readable-stream is commonjs

ljharb commented 4 years ago

I don't see why it would be - i don't know how jspm.dev works but i assume it's a bundler, and there's zero reason it wouldn't be able to bundle CJS as well.

vweevers commented 4 years ago

The goals set by @mcollina in the OP are already ambitious, let's stop there. Webpack 5 users have a way to keep using readable-stream, other bundlers should offer similar compatibility (to pursue their goals). Otherwise readable-stream can't maintain its compatibility with node, browserify and the npm ecosystem. Plus, module authors will have enough work in upgrading from v3 to v4.

bergos commented 4 years ago

Replace commonjs with modules.

I think it should not be replace, but add ESM support.

Otherwise readable-stream can't maintain its compatibility with node, browserify and the npm ecosystem.

Making it a dual package should not cause any problems.

i don't know how jspm.dev works but i assume it's a bundler

jspm.dev is still a bundler, but I think we are heading towards the possibility to use ESM without a bundler, fetched from plain CDN. Yes, this will cause a lot of requests and yes, a serious web application should be bundled for better user experience. But if readable-stream doesn't add this feature now, it will block a lot of other packages moving that direction.

Some context: I'm involved in a lot of RDF packages. Many of them rely on readable-stream as the RDF/JS stream interfaces spec uses a compatible interface. I write mainly code for node, which makes things easy-peasy for me. But many people that want to use the libraries are not the most experienced JS developers and have no idea of bundlers. They favor libraries that can be imported with a <script> element. For a while I maintained a dist builder, but it has a lot of limitations. ESM would give them almost the same user experience.

I'm not an browser ESM expert, so I don't know if import maps can solve already the problem of finding the entry point of an npm package. If not, I'm sure soon there will be a solution for it. Maybe it's just a property in package.json or an addition file in the package, but I guess it would be only a feature version if the package is already provided as ESM.

timoxley commented 3 years ago

Note there are currently many weird, subtle problems using async iteration/generators with the 3.x version of readable-stream that just aren't there for node v14 streams, would be great if this could get updated soon.

thekevinbrown commented 3 years ago

Also a note on the Webpack v5 controversy, Vite also does not provide Node shims, and it's really painful to get it to work.

endyjasmi commented 3 years ago

Hello, any news on this? what can I do to make this happen.

thekevinbrown commented 2 years ago

If you want a version without circular dependencies, I forked and published this: https://www.npmjs.com/package/vite-compatible-readable-stream

If you want something else, not sure.

tpluscode commented 2 years ago

@thekevinbrown maybe this deserves to be merged into a 3.x release so that we're not stuck with webpack?

thekevinbrown commented 2 years ago

@tpluscode happy to PR it here if it’ll be accepted. There was an issue somewhere where most of the conversation was about how this project likes to consume what comes from Node core directly so I didn’t think it was likely to be a welcome direction.

souljorje commented 2 years ago

@thekevinbrown thanks, your package helped a lot!

timoxley commented 2 years ago

wow đź’–