Fix/remove node globals - rebased & fixing IE issue

[martinheidegger]

Rebased version of #435 with patch to fix IE.

[mcollina]

@vweevers @ljharb wdyt?

[ljharb]

I think that this approach guarantees that most folks will have more than one nextTick shim in their bundle, which will inflate bundle sizes far more than just fixing webpack 5’s broken behavior.

I don’t understand why any change is being made here when webpack 5’s choice was to force users to deal with this. I also don’t see how it’s sustainable to try to force every package in the ecosystem to make similar changes, nor why this will be a benefit to the ecosystem to now have dozens of duplicate shims that are impossible to deduce (because they’re all potentially subtly different).

[ljharb]

it’s very slow to use new Array; why is this not slicing arguments into an array?

[martinheidegger]

Indeed. I updated it for performance and push-rebased: https://github.com/nodejs/readable-stream/pull/454/files#diff-773895e366710aaad05c2daf697abc214727c9de1227114b5694878bd33322b2

whoops, sorry, didn’t mean to make it be a red X

[martinheidegger]

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]

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]

@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:

• We can impose the cost on the users.
• We can take the burden on ourselves and provide users with a better experience.

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

[ljharb]

@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]

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]

@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]

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

[ljharb]

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]

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