Fix/remove node globals

[hugomrdias]

This PR removes usage of node globals and adds buffer, events and next-tick to the dependencies. This will allow readable-stream to works with bundlers that don't inject node globals.

Some code is borrowed from this PR https://github.com/nodejs/readable-stream/pull/414

Warning: These lines should make the browsers tests fail https://github.com/nodejs/readable-stream/blob/master/lib/internal/streams/buffer_list.js#L18-L19 https://github.com/nodejs/readable-stream/blob/master/lib/internal/streams/buffer_list.js#L200 because of this https://github.com/nodejs/readable-stream/blob/master/package.json#L55 but they don't, so either airtap ignores 'util': false or something else is happening that i didn’t catch. I need some feedback on how you guys would like to fix this, maybe use https://www.npmjs.com/package/browser-util-inspect or just mock inspect.

[hugomrdias]

@vweevers i used next-tick instead of immediate (this one made one test fail) and it doesn't use global maybe we can switch abstract-leveldown to it ?

[vweevers]

At fist glance, next-tick doesn't have a queue and doesn't support args (process.nextTick(fn,1,2)).

[hugomrdias]

calvinmetcalf/process-nextick-args

This module uses process.nextTick the goal of this PR is to NOT use process.nextTick.

Btw where are nextTick positional arguments used? If this is really the case maybe we need to add a test for that because everything is green now.

[hugomrdias]

@mcollina can you have a look at the warning i wrote in the PR description

[mcollina]

can you have a look at the warning i wrote in the PR description

I agree, there is something weird going on. @vweevers do you think we can disable node globals with airtap?

[vweevers]

@hugomrdias Are you sure that this line is called by any browser test?

https://github.com/nodejs/readable-stream/blob/7eded7bc298b14d1960e1db9ee48450bc61085ee/lib/internal/streams/buffer_list.js#L200

If so, what is the actual value of inspect that you see? The other line isn't expected to fail, because util: false means that require('util') returns an empty object:

https://github.com/nodejs/readable-stream/blob/7eded7bc298b14d1960e1db9ee48450bc61085ee/lib/internal/streams/buffer_list.js#L18-L19

[vweevers]

@mcollina We can specify browserify options via .airtap.yml, along the lines of:

browserify:
  options:
    detectGlobals: false

Not sure if you mean globals (like process) or core modules (like util) though.

[hugomrdias]

@hugomrdias Are you sure that this line is called by any browser test?

https://github.com/nodejs/readable-stream/blob/7eded7bc298b14d1960e1db9ee48450bc61085ee/lib/internal/streams/buffer_list.js#L200

yeah i did some more testing and this line is never called in the browser tests

If so, what is the actual value of inspect that you see? The other line isn't expected to fail, because util: false means that require('util') returns an empty object:

https://github.com/nodejs/readable-stream/blob/7eded7bc298b14d1960e1db9ee48450bc61085ee/lib/internal/streams/buffer_list.js#L18-L19

and yes the require2 is {} but inspect is undefined we should at least add a guard at the bottom to future proof

[hugomrdias]

@mcollina We can specify browserify options via .airtap.yml, along the lines of:

browserify:
  options:
    detectGlobals: false

Not sure if you mean globals (like process) or core modules (like util) though.

it would be everything so we can be sure this works with webpack 5 or rollup without node plugin, etc. the problem here is all the tests would break because they come directly from node core.

im not seeing a way to test this properly with the current setup, i can only validate this in ipfs/libp2p tests (where i can disable node stuff for the full test bundle) and report back any issue i find.

[mcollina]

it would be everything so we can be sure this works with webpack 5 or rollup without node plugin, etc. the problem here is all the tests would break because they come directly from node core.

This is not correct, we run different tests in the browser: https://github.com/nodejs/readable-stream/tree/master/test/browser. Also this is important https://github.com/nodejs/readable-stream/blob/7eded7bc298b14d1960e1db9ee48450bc61085ee/test/browser.js#L1-L28.

I think we should update the browser test environment so that we do not rely on node core stuff.

[vweevers]

im not seeing a way to test this properly with the current setup, i can only validate this in ipfs/libp2p tests (where i can disable node stuff for the full test bundle)

This should do the trick:

browserify:
  - options:
      detectGlobals: false
      builtins: false

[hugomrdias]

even if the tests code can run without node builtins how would tape run ?

builtins: false gives me this

Uncaught Error: Cannot find module '_process'
    at o (_prelude.js:1)
    at _prelude.js:1
    at Object./Users/hugomrdias/code/readable-stream/node_modules/tape/index.js../lib/default_stream (index.js:151)
    at o (_prelude.js:1)
    at _prelude.js:1
    at Object.<anonymous> (browser.js:13)
    at Object./Users/hugomrdias/code/readable-stream/test/browser.js../browser/test-stream-big-packet (browser.js:82)
    at o (_prelude.js:1)
    at r (_prelude.js:1)
    at _prelude.js:1

[mcollina]

Ok, this is getting complicated. Before supporting this use case, we'd need a way to automatically verify this works at a very high level.

I'm happy to switch frontend framework if it helps

[vweevers]

Alternatively, fix tape to work without globals as well (readable-stream isn't the only module that will hit this problem). /cc @ljharb do you reckon that's feasibe?

[ljharb]

Bundlers that don’t inject node globals when bundling node modules are broken. The fix is to fix or replace your bundler, not attempt to add complexity to every module in the ecosystem.

[vweevers]

@ljharb That's a discussion to take up with Webpack. I can kinda see their point but it's putting a strain on the ecosystem and its maintainers. Wish I had the time to engage Webpack folks.

[mcollina]

Is there a flag or plugin that can be passed to webpack to do this?

[ljharb]

@vweevers those who it's putting a strain on, are who should be taking up that discussion.

[vweevers]

@mcollina There's no flag, because webpack 5 removes it:

[vweevers]

I see a couple of options:

1. Take a principled stance that bundlers must inject node globals. If nothing changes on webpack's side, this option will alienate webpack users.
2. Remove node globals here, without the ability to verify it works. We'd be supporting webpack users in theory only; things can break without us knowing.
3. Remove node globals here, run browser tests with webpack and alternatives to airtap and tape.

[ljharb]

Webpack users will already likely stay on v4, since v5's choices break the ecosystem. I think the stance that "a bundler for X to environment Y should work for any code written for X that can possibly work in environment Y" isn't an unreasonable one to take.

[vweevers]

I agree that's a reasonable stance. If we take it, I do believe further action is required. Because this PR and many others prove that "webpack users will already likely stay on v4" isn't true; webpack is actively encouraging folks to update modules like readable-stream and is not offering alternative solutions atm.

[ljharb]

@vweevers i had some discussion on the webpack slack, and it looks like https://github.com/webpack/changelog-v5#automatic-nodejs-polyfills-removed tells you how to restore this behavior using the ProvidePlugin, without needing to alter any part of the ecosystem.

[vweevers]

@hugomrdias Could you try that out? https://webpack.js.org/plugins/provide-plugin/

[hugomrdias]

@vweevers thats not an option, i would need to teach every end user how to do that.

I updated the PR with simple, maintainable browser overrides, this way we are sure airtap is using the overrides and all tests are green.

Looking forward for your feedback.

[ljharb]

@hugomrdias every webpack 5+ user will already need to learn how to do that; there are too many modules in the ecosystem to be updated before they wouldn’t have to (even assuming you could update all of them in this way).

[hugomrdias]

Sure maybe, and it's going to be a huge mess and issues will pile up everywhere. At least readable-stream could be easy to use, no extra config needed.

[ljharb]

The place to fix that mess is the place that caused it, webpack, not in individual pieces of the massive npm ecosystem ¯\_(ツ)_/¯

[vweevers]

issues will pile up everywhere

If modules in the ecosystem end up with different nexttick replacements, there will be nastier issues, like subtle timing differences, unpredictable order of events, and degraded performance.

[hugomrdias]

Exactly why I just used queueMicrotask and authors shouldn't leave the decision to the end users or the bundlers.

[calvinmetcalf]

there are quite a few subtle corner cases that can show up when rolling your own next tick replacement, (source, have done so, wrote most of the process.nextTick currently in browserify) you may want to consider that or something like immediate by me, asap or something more recent since I've been somewhat out of the game lately. The browerify process.nextTick one handles all the weird corner cases like

• like people trying to remove setTimeout to make all promises synchronous
• the queued function calls window.open which in older firefox would stops the current function from running but does not stop other timers from firing.
• weird IE bugs caused by setTimeout having a different context object that only popup if the code is being dynamically evaled
• globals being defined latter and not on initial code run

[ljharb]

Authors are never going to use a single shim - every app probably has two object.assign packages, for example. That problem can only be fixed in the bundler config. Forcing node builtin shim choice into the ecosystem worsens that problem, and forces more configuration into the end user’s bundler config.

[hugomrdias]

I would like to not turn this thread into a discussion about if webpack is doing the correct thing or not.

This PR doesnt actually shim anything it just uses the microtask queue thats available in browser. It only provides the node version (nextTick) because of the tests that only run in node.

[vweevers]

it just uses the microtask queue thats available in browser

The problem is the same: other modules don't use this particular solution.

I suppose ultimately they should but many modules (including readable-stream) still support IE, and IE does not support queueMicrotask or the Promise-based fallback.

[hugomrdias]

Really IE ? Shouldn’t we drop that ?

To support it the only solution is to use https://github.com/calvinmetcalf/immediate but this only guarantees that the internals use the same queue.

Should we just use immediate or add a third fallback queueMicrotask > Promise > immediate or just queueMicrotask > immediate?

[mcollina]

We'll drop IE in the next major, which we should be starting working on asap tbh :D. I'll open an issue.

[mcollina]

https://github.com/nodejs/readable-stream/issues/439

[calvinmetcalf]

if we want a concrete example of the problems of using different microtask shims (ironically, all written by me) see this issue defunctzombie/node-process#88

[martinheidegger]

In regards to tape: in my fork https://github.com/martinheidegger/tape I updated it to use no node-globals. It references in the dependencies back to this branch https://github.com/martinheidegger/tape/blob/5c2a1ebeabda328539dbd117a369ff91de9701d6/package.json#L38-L43 Outdated.

[martinheidegger]

As tape is committed to stay version compatible (see: #505 and #506), it seemed to me a bit tricky to make sure this works, so I decided to fork tape → fresh-tape that compiles with webpack. Currently fresh-tape depends to this branch, and on through2#102. As such, I would love to see this branch merged.

[ljharb]

I would urge this package to also retain support for older nodes.

[martinheidegger]

@ljharb If I was able to make it support node < 8 I would have just sent PR's to tape...

[ljharb]

@martinheidegger i mean, concat-stream could still be changed to make that possible :-) but really the PRs should be sent to webpack.

[martinheidegger]

@ljharb concat-stream might work but through should be tricky, tap is very unlikely (though may also be unnecessary); additionally I am not sure about the compatibility of all new dependencies (like through2, inherits, path-browserify, ...)... In short: if you manage to get it to work I am happy to deprecate/archive fresh-tape in favor of tape...

Personally I am in favor of webpack enforcing a no-node-globals policy as this is probably good for the ecosystem - big picture, but either way I will adapt to what is given.

[kelly-tock]

is there any update on this? seems like it would fix quite a few issues with webpack 5.

[pkit]

This PR removes usage of node globals and adds buffer, events and next-tick to the dependencies. This will allow readable-stream to works with bundlers that don't inject node globals.

Some code is borrowed from this PR #414

Warning: These lines should make the browsers tests fail https://github.com/nodejs/readable-stream/blob/master/lib/internal/streams/buffer_list.js#L18-L19 https://github.com/nodejs/readable-stream/blob/master/lib/internal/streams/buffer_list.js#L200 because of this https://github.com/nodejs/readable-stream/blob/master/package.json#L55 but they don't, so either airtap ignores 'util': false or something else is happening that i didn’t catch. I need some feedback on how you guys would like to fix this, maybe use https://www.npmjs.com/package/browser-util-inspect or just mock inspect.

Any non-node env will fail when stream is piped couple of times, like stream = stream.pipe(new PassThrough(stream)).pipe(new PassThrough(stream)) Webpack obviously produces failing code there:

var _require2 = __webpack_require__(/*! util */ "?1dff"),
    inspect = _require2.inspect;

/***/ "?1dff":
/*!**********************!*\
  !*** util (ignored) ***!
  \**********************/
/***/ (() => {

/* (ignored) */

/***/ }),

P.S. will fail in any other bundler I know, btw.

[reintroducing]

This package is a dependency of microphone-stream which is not working when using Vite. I believe this is cause by the use of global as seen in the screenshot of my bug report in their repo. Is the removal of the node globals something that can be handled or is it not possible, until a rewrite, to use this package in the Vite ecosystem?

[ljharb]

@reintroducing the proper vix remains that vite, webpack 5, and whoever else has made this incorrect choice but claims to be able to bundle node modules, needs to provide, or transform global.

Vite (or anyone else) could very easily transform every global instance to globalThis by default, and it would Just Work for everyone, and not require any upstream changes.

[reintroducing]

@ljharb I've read the exchanges between you and others and engineers on the Vite team in various GH issues over the past few days while trying to find a way around this. it's an unfortunate reality that we are currently in but nonetheless a reality that exists. that being said, i cannot find a way to do this in Vite, nor have I found any sort of polyfill for it, so am I basically left with the reality that I either use webpack 4 with my codebase (that would be very unfortunate and not really an option for us realistically) or continue to use webpack 5/Vite and find a different way to achieve what I'm after in my particular code (basically have to find a replacement for microphone-stream)? I have been searching but can't find a way to polyfill this in vite (and realistically how to do it in webpack 5 is also not something I've come across successfully).

[ljharb]

I would suggest avoiding tools that fail to interoperate with the existing, decade-old ecosystem and conventions.