Check for existence of `process` object (fix for v2)

[th0r]

Fixes https://github.com/nodejs/readable-stream/issues/412

Notes: This PR fixes v2 and was branched from v2.3.6 tag so it would be great to ship v2.3.7 with this fix.

[th0r]

@mcollina @calvinmetcalf is there a chance you could look at it?

[mcollina]

Sorry for the long delay! The change in itself looks fine, however we need something slightly different for this to be released.

This library currently is extracted from node core (version 10) via a series of scripts in the build/ folder. Would you mind tweaking this build step to apply these changes?

It would need a test as well. Would you mind adding it in test/browser?

Thanks!

[th0r]

Would you mind tweaking this build step to apply these changes?

@mcollina Done.

It would need a test as well. Would you mind adding it in test/browser?

I'm not sure what exactly to test. Also, I couldn't find a place where browser tests are actually being started - test script in package.json runs only tests under parallel and ours subdirs. Shall I use npx tape test/browser command? But in this case tests are run in Node.js environment and global process object is defined there.

[vweevers]

@th0r regarding browser tests: in CI we're doing npm run test-browsers which runs tests in Sauce Labs browsers. Locally you can do npm run test-browser-local to run tests in a browser of choice.

[th0r]

@vweevers as I noted in PR description it's a fix for readable-stream@2 and PR was branched from v2.3.6 tag. I can't find neither test-browsers nor test-browser-local npm scripts there.

[vweevers]

Ah sorry, I missed that, nevermind me then :)

[mcollina]

Is this not needed in v3?

[vweevers]

You could try npx airtap --open --local -- test/browser.js in 2.x.

[th0r]

@vweevers yes, it works (and they are all green) but do you run browser test at all on CI for v2?

[th0r]

And another thought: maybe you could create a public v2 branch from v2.3.6 tag so I could set it as a target for this PR?

[vweevers]

yes, it works, but do you run browser test at all on CI?

At that time readable-stream did not have browser tests because they were too unstable (edit to be precise: the tooling was unstable, not the tests themselves). We could copy the setup from 3.x to 2.x, but I'm not sure that's worth the trouble.

[th0r]

Well, all existing tests pass in Chrome v75 / FF 67 / Safari 12.1.1

[th0r]

@vweevers @mcollina so what are your thoughts guys?

[mcollina]

I still did not get an answer to: https://github.com/nodejs/readable-stream/pull/413#issuecomment-507216010

Is this not needed in v3?

[th0r]

I still did not get an answer to

Can @vweevers's comment and my comment be treated as an answer?

[mcollina]

Not at all.

Does readable-stream v3 suffer from the same issue? I'm not keen on having the two versions diverge in behavior.

[th0r]

Does readable-stream v3 suffer from the same issue?

According to this line it does: https://github.com/nodejs/readable-stream/blob/4ba93f84cf8812ca2af793c7304a5c16de72088a/lib/_stream_readable.js#L609

I can fix it in a separate PR.

[mcollina]

let's do that, thanks!

[th0r]

Could you create and push a v2 branch so I could set it as a target for this PR? You're fast!

[th0r]

let's do that, thanks!

@mcollina Tried to do the same for v3 but seems like you're relying on process.nextTick and don't use any polyfills.

All browser tests pass because airtap uses browserify to bundle browser tests which in turn polyfills process object including process.nextTick.

I'm not sure what to do. Even if we replace process.nextTick with some internal polyfill I can't see any way to test that we don't rely on global process object because it will always be provided by the airtap itself.

[vweevers]

Even if we replace process.nextTick with some internal polyfill

Assuming we want to do that (I have no opinion on it), then one way to run tests without a process global would be to pass browserify options to airtap (via .airtap.yml) to disable the shimming of process.

[th0r]

Assuming we want to do that (I have no opinion on it)

I would do that because webpack 5 will remove auto-polyfilling of Node internals/global objects by default including process object. I tried to bundle readable-stream with it and it resulted in a runtime error process is not defined.

[vweevers]

webpack 5 will remove auto-polyfilling of Node internals/global objects by default

🙀 That's a scary move for the ecosystem. For those reading along, see Automatic Node.js Polyfills Removed.

[th0r]

So, we have 2 options:

1. Use some external library instead of process.nextTick e.g. immediate. It tries to be as close to original implementation as possible.
2. Write our own tiny polyfill that uses setTimeout(0) or borrow it's implementation from the webpack.

Pros for №1:

• Very close to spec
• More performant

Cons:

• Larger that №2
• May affect current behavior of readable-stream in the browser as it's more close to the spec (breaking change?)

Pros for №2:

• Small size
• No extra dependency
• Doesn't break current behavior in the browser (if we'll borrow webpack's implementation)

Cons:

• Behaves differently than original process.nextTick
• Slower

@mcollina @vweevers thoughts?

[vweevers]

-1 on changing behavior, for reasons mentioned in https://github.com/webtorrent/webtorrent/issues/1568#issuecomment-453682828. To my understanding the webpack change is not final; perhaps some pushback is in order.

[th0r]

@vweevers if I understand you correctly, you don't want to add a spec-compliant dependency to polyfill nextTick? But what do you suggest then? Make our own implementation? Use webpack's version? Or mock the whole process object using browserify's polyfill?

[mcollina]

Just to clarify this issue, because it's still not clear to me. Is this problem happening only with Webpack v5? Or is this happening in some other cases? If current browserify and webpack inject process, how could you have ended up with this bug?

@vweevers if I understand you correctly, you don't want to add a spec-compliant dependency to polyfill nextTick? But what do you suggest then? Make our own implementation? Use webpack's version? Or mock the whole process object using browserify's polyfill?

I think we should create a tiny nextTick.js file in this repo, and ship that within our module. This should match browserify/webpack behavior so it's not a breaking change. We might want to consider alternatives for readable-stream v4.

[th0r]

If current browserify and webpack inject process, how could you have ended up with this bug?

Both browserify and webpack have config options to disable injection of Node.js internals. We use webpack with such option (config = {node: {process: false}}) to workaround Angular bug. I explained it all in the issue.

[mcollina]

Both browserify and webpack have config options to disable injection of Node.js internals. We use webpack with such option (config = {node: {process: false}}) to workaround Angular bug. I explained it all in the issue.

I think it should be possible to configure airtap to not expose the process object. So we can test this in v3.

[th0r]

I think it should be possible to configure airtap to not expose the process object.

Yes, it's possible by adding these lines to .airtap.yml:

browserify:
  - options:
      detectGlobals: false
      insertGlobals: false

But in this case npm run test-browser-local fails with ReferenceError: global is not defined caused by these lines in test/browser.js: https://github.com/nodejs/readable-stream/blob/4ba93f84cf8812ca2af793c7304a5c16de72088a/test/browser.js#L1-L12

If I remove them it fails with the same global is not defined exception, which occurs in the buffer npm package that is required as result of this require call: https://github.com/nodejs/readable-stream/blob/4ba93f84cf8812ca2af793c7304a5c16de72088a/test/browser.js#L13 Here is the whole "require chain": test/browser.js => tape => through => stream-browserify => readable-stream@2.3.6 (yes, exactly this library 😁) => safe-buffer => buffer.

If I add window.global = {} at the beginning of the test/browser.js then it fails with ReferenceError: Buffer is not defined which is caused by core-util-is module in the following require chain: test/browser.js => tape => through => stream-browserify => readable-stream@2.3.6 => core-util-is.

So, I'm out of ideas how to make browser tests work without polyfilling Node.js globals and seems like process object is not the only problem here.

[th0r]

Seems like this package also relies on the buffer and events polyfills but they're not present in the dependencies list: https://github.com/nodejs/readable-stream/blob/4ba93f84cf8812ca2af793c7304a5c16de72088a/lib/_stream_writable.js#L70 https://github.com/nodejs/readable-stream/blob/4ba93f84cf8812ca2af793c7304a5c16de72088a/lib/internal/streams/stream-browser.js#L1 Currently it works because webpack provides polyfills for them automatically but it will break in webpack 5.

[vweevers]

I suggest to split this discussion, because supporting an undefined process on v2 has been achieved here, but doing the same on v3 as well as accounting for webpack 5 is a bigger task / discussion.

[mcollina]

Can we get a similar PR for v3 (even without tests)?

[th0r]

Can we get a similar PR for v3

Shall I borrow webpack's implementation? It's quite primitive...

[mcollina]

Shall I borrow webpack's implementation? It's quite primitive...

I don't understand the question. This PR is not borrowing anything from Webpack.

[th0r]

This PR didn't need to replace usages of process.nextTick, but in v3 I need to do this.

[mcollina]

I don't understand why.

[th0r]

Because v2 used process-nextick-args for this purpose. And I don't know why v3 decided not to use it anymore.

[mcollina]

process-nextick-args  was used to support old versions of Node. You can lift some of that code and add it to this repo.

[th0r]

Appears that process-nextick-args also uses global process object under the hood - it just adds support for additional arguments. So process-nextick-args can't be used in the browser as well.

I can do the following:

For v2:

1. Add ./process-browser.js with the following content:

   
   module.exports = {
   nextTick: nextTick
   };

function nextTick(fn) { var args = Array.prototype.slice.call(arguments, 1); setTimeout(function () { fn.apply(null, args); }, 0); }

2. Add the following config to `package.json`:
```json
{
  "browser": {
    "process-nextick-args": "./process-browser.js"
  }
}

For v3:

1. Add ./process.js with the following content:

   module.exports = process;

2. Add the same ./process-browser.js as in v2
3. Add var process = require('../process') replacements in all files that use global process object.
4. Add the following config to package.json:

   {
   "browser": {
   "./process": "./process-browser.js"
   }
   }

   @mcollina thoughts?

[mcollina]

I think it's cleaner to add var nextTick = require('./next-tick.js').nextTick in both versions, and then use your ponyfill for the browser. In readable-stream@2, we will still use process-nextick-args in there.

Does it make sense?

[th0r]

I think it's cleaner to add var nextTick = require('./next-tick.js').nextTick in both versions

Why do we need this nextTick in v2 if you say that we'll still use process-nextick-args there?

[mcollina]

Why do we need this nextTick in v2 if you say that we'll still use process-nextick-args there?

True, it's not needed.

[th0r]

@mcollina @vweevers created a PR for v3: #414

[th0r]

@mcollina may I expect it to be merged and published or shall I better search for another solution to my problem?

[mcollina]

Sure thing, it’s sitting in my list of things to do. I had to take some unexpected time off recently and I’m falling behind.

[th0r]

@mcollina any news?

[kostos]

Any ETA when this can be merged?