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 #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)).

[mcollina]

In the past we used https://github.com/calvinmetcalf/process-nextick-args instead. The reason we dropped was to remove a dependency (because there is a phobia of having too many dependencies).

next-tick would not work for this module because it does not support positional arguments.

[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.

[mcollina]

This should not work with the polyfill. I think our browser testing is adding process.nextTick.

[hugomrdias]

its just using the first if here https://github.com/medikoo/next-tick/blob/master/index.js#L47-L49 because airtap is injecting everything, its hard to validate this with the current repo setup i will try to fix this

[mcollina]

Yes exactly. However if those transform are not put in there, it won't work.

[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?

readable-stream/lib/internal/streams/buffer_list.js

Line 200 in 7eded7b

return inspect(this, _objectSpread({}, options, {

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:

readable-stream/lib/internal/streams/buffer_list.js

Lines 18 to 19 in 7eded7b

	var _require2 = require('util'),
	inspect = _require2.inspect;

[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?

readable-stream/lib/internal/streams/buffer_list.js

Line 200 in 7eded7b

return inspect(this, _objectSpread({}, options, {

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:

readable-stream/lib/internal/streams/buffer_list.js

Lines 18 to 19 in 7eded7b

	var _require2 = require('util'),
	inspect = _require2.inspect;

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

readable-stream/test/browser.js

Lines 1 to 28 in 7eded7b

	if (!global.console) {
	global.console = {};
	}
	if (!global.console.log) {
	global.console.log = function () {};
	}
	if (!global.console.error) {
	global.console.error = global.console.log;
	}
	if (!global.console.info) {
	global.console.info = global.console.log;
	}
	var test = require('tape');
	var util = require('util');
	// TODO: add replacements instead
	global.process = {
	env: {},
	on: function () {},
	cwd: function () {
	return '/';
	},
	binding: function () {
	return {
	hasTracing: false
	};
	}
	};

.

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/

[mcollina]

#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.

[targos]

I've been using this in my Vite config to work around a module that uses global:

    define: {
      global: 'globalThis',
    },

[reintroducing]

@targos amazingly that worked...! is it really that simple? I feel like it can't be, but if it is, im kind of shocked.

[reintroducing]

Lol, so after getting that working, I got to my next roadblock, which, in a production build of Vite, buffer.from is not available (as a result of another dependency of readable-stream,safe-buffer) . That led me down the path of trying to find a solution for that, which I still have yet to do, but i found another workaround for the global stuff, which was adding this in my vite config file:

import NodeModulesPolyfills from '@esbuild-plugins/node-modules-polyfill';

optimizeDeps: {
    esbuildOptions: {
        plugins: [NodeModulesPolyfills()]
    }
}

I tried adding to that the following:

import GlobalsPolyfills from '@esbuild-plugins/node-globals-polyfill'

optimizeDeps: {
    esbuildOptions: {
        plugins: [
            GlobalsPolyfills({
                buffer: true,
            }),
            NodeModulesPolyfills(),
        ],
    },
},

but of course that didn't work. I'm just documenting all of this here in case someone else lands on this in a google search eventually and is facing these issues to save them some time.

I feel like this is going to be a fun little game of cat and mouse until I can get the production build working for all dependencies.

[reintroducing]

was finally able to get the production build working (with global and buffer) using the following config:

import nodePolyfills from 'rollup-plugin-polyfill-node';
import NodeModulesPolyfills from '@esbuild-plugins/node-modules-polyfill';

resolve: {
    alias: {
        // don't need to add this to deps, it's included by @esbuild-plugins/node-modules-polyfill
        buffer: 'rollup-plugin-node-polyfills/polyfills/buffer-es6',
    },
},
optimizeDeps: {
    esbuildOptions: {
        plugins: [
            NodeModulesPolyfills(),
        ],
    },
},
build: {
    rollupOptions: {
        plugins: [nodePolyfills()],
    },
},

While this works, i have absolutely ZERO confidence in it in not only its current state but when updates to Vite (or any of these plugins) are made in the future.