nodejs / readable-stream

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

Fix/remove node globals - rebased & fixing IE issue #454

Closed martinheidegger closed 3 years ago

martinheidegger commented 3 years ago

Rebased version of #435 with patch to fix IE.

mcollina commented 3 years ago

@vweevers @ljharb wdyt?

martinheidegger commented 3 years ago

I think that this approach guarantees that most folks will have more than one nextTick shim in their bundle.

Hmm, would it be okay to use/require queueMicrotask with a fallback to nextTick until we drop support for Node 10? It is quite well established and there are polyfills? caniuse#queueMicrotask

ljharb commented 3 years ago

I think that anything that's in node itself shouldn't be provided, and that it is the exclusive job of a node module bundler to provide shims for node builtins in the browser. In other words, this package can and should do whatever's needed to support the desired node versions, only. If webpack 5 has abdicated part of its only job, that shouldn't impose a cost on the entire javascript ecosystem (not just the part using webpack 5).

benjamingr commented 3 years ago

@ljharb

If webpack 5 has abdicated part of its only job, that shouldn't impose a cost on the entire javascript ecosystem (not just the part using webpack 5).

Webpack already did and the cost was already imposed.

Now maintainers have two choices:

Given we already have a volunteer PR for #2 and Martin spent the work picking up the slack - why not take this code?

ljharb commented 3 years ago

@benjamingr because webpack users - 5 or otherwise- aren’t the only, or the most important, set of users in the ecosystem. This PR imposes a cost to everyone (or potentially ever other bundler user, depending on how it’s authored), and that is not a reasonable thing to inflict.

Don’t forget about option 3 - we can hold the line, and webpack 5 may eventually fix its mistakes.

martinheidegger commented 3 years ago

I was actually trying to suggest option 4: use the cross-platform queueMicrotask() instead of a platform specific process.nextTick and supply fallbacks for old version that don't support queueMicrotask but that we want to be supported. For tests sake I tried it out on this branch but I couldn't get the tests to work (Would appreciate hints on how to fix them).

vweevers commented 3 years ago

@martinheidegger I raised that idea earlier and @mcollina responded:

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

Can we try to prove that statement and then think about solutions? Because in packages where node builtins can safely be replaced with browser builtins, Webpack's change can indeed have a positive impact. That is not the case here - as far as we know now. Shims are a trap because they provide a better (build) user experience at first but hurt in the long run (1, 2, 3, 4). So I prefer to hold the line while we do explore long-term alternatives.

For example, one difference between node's queueMicrotask and nextTick is that the queueMicrotask queue is processed after the nextTick queue. What are the implications of that (in userland, beyond breaking very specific expectations in our tests)?

martinheidegger commented 3 years ago

@mcollina also mentioned that it would be on us to support Webpack 5. I think this current PR is the best possible solution.

ljharb commented 3 years ago

Note that a viable strategy to support webpack 5 is to simply link to instructions of how to use ProvidePlugin to effectively revert webpack 5’s harmful change.

martinheidegger commented 3 years ago

I agree with you, updating this is too much of a mess.