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

th0r · 2019-06-20T09:20:24Z

Fixes #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 · 2019-06-24T16:56:47Z

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

mcollina · 2019-06-29T06:06:24Z

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 · 2019-07-01T09:59:17Z

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 · 2019-07-01T10:45:08Z

@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 · 2019-07-01T10:48:41Z

@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 · 2019-07-01T10:50:28Z

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

mcollina · 2019-07-01T10:53:43Z

Is this not needed in v3?

vweevers · 2019-07-01T10:53:47Z

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

th0r · 2019-07-01T10:56:46Z

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

th0r · 2019-07-01T10:59:38Z

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 · 2019-07-01T11:01:22Z

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 · 2019-07-01T11:15:12Z

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

th0r · 2019-07-02T12:35:00Z

@vweevers @mcollina so what are your thoughts guys?

mcollina · 2019-07-04T13:00:51Z

I still did not get an answer to: #413 (comment)

Is this not needed in v3?

th0r · 2019-07-04T13:03:21Z

I still did not get an answer to

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

mcollina · 2019-07-04T13:05:59Z

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 · 2019-07-04T13:10:23Z

Does readable-stream v3 suffer from the same issue?

According to this line it does:

readable-stream/lib/_stream_readable.js

Line 609 in 4ba93f8

    
           var doEnd = (!pipeOpts || pipeOpts.end !== false) && dest !== process.stdout && dest !== process.stderr;

I can fix it in a separate PR.

mcollina · 2019-07-04T13:10:57Z

let's do that, thanks!

th0r · 2019-07-04T13:12:12Z

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

th0r · 2019-07-04T15:14:41Z

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 · 2019-07-05T06:22:33Z

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 · 2019-07-05T08:12:41Z

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 · 2019-07-05T08:23:04Z

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 · 2019-07-05T08:50:48Z

So, we have 2 options:

Use some external library instead of process.nextTick e.g. immediate. It tries to be as close to original implementation as possible.
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 · 2019-07-05T09:02:31Z

-1 on changing behavior, for reasons mentioned in webtorrent/webtorrent#1568 (comment). To my understanding the webpack change is not final; perhaps some pushback is in order.

th0r · 2019-07-05T09:24:17Z

@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 · 2019-07-05T11:35:30Z

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 · 2019-07-05T11:39:46Z

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 · 2019-07-05T11:43:58Z

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 · 2019-07-05T12:11:30Z

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:

readable-stream/test/browser.js

Lines 1 to 12 in 4ba93f8

    
           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; 
        
           }

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:

readable-stream/test/browser.js

Line 13 in 4ba93f8

var test = require('tape');

Here is the whole "require chain":

test/browser.js => tape => through => stream-browserify => [email protected] (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 => [email protected] => 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 · 2019-07-05T12:26:30Z

Seems like this package also relies on the buffer and events polyfills but they're not present in the dependencies list:

readable-stream/lib/_stream_writable.js

Line 70 in 4ba93f8

var Buffer = require('buffer').Buffer;

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

Line 1 in 4ba93f8

module.exports = require('events').EventEmitter;

Currently it works because webpack provides polyfills for them automatically but it will break in webpack 5.

vweevers · 2019-07-05T13:02:38Z

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 · 2019-07-05T13:07:27Z

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

th0r · 2019-07-05T13:22:50Z

Can we get a similar PR for v3

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

mcollina · 2019-07-05T13:31:14Z

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 · 2019-07-05T13:32:33Z

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

mcollina · 2019-07-05T13:33:45Z

I don't understand why.

th0r · 2019-07-05T13:34:51Z

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

mcollina · 2019-07-05T13:39:16Z

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 · 2019-07-05T14:00:45Z

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:

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);
}

Add the following config to package.json:

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

For v3:

Add ./process.js with the following content:

module.exports = process;

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

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

@mcollina thoughts?

mcollina · 2019-07-05T14:26:25Z

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 · 2019-07-05T14:31:08Z

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 · 2019-07-05T15:10:45Z

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 · 2019-07-08T11:24:56Z

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

th0r · 2019-07-22T10:30:29Z

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

mcollina · 2019-07-22T10:46:09Z

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 · 2019-08-02T09:06:23Z

@mcollina any news?

kostos · 2019-08-06T07:32:13Z

Any ETA when this can be merged?

rsbondi · 2019-09-13T17:01:26Z

lib/_stream_writable.js

@@ -54,7 +54,7 @@ function CorkedRequest(state) {
 /* </replacement> */

 /*<replacement>*/
-var asyncWrite = !process.browser && ['v0.10', 'v0.9.'].indexOf(process.version.slice(0, 5)) > -1 ? setImmediate : pna.nextTick;
+var asyncWrite = typeof process !== 'undefined' && !process.browser && ['v0.10', 'v0.9.'].indexOf(process.version.slice(0, 5)) > -1 ? setImmediate : pna.nextTick;


I have a problem with process.version also undefined, so && typeof process.version !== 'undefined' check also suggested, fyi nativescript is the environment

Check for existence of process object

fd4dda7

th0r mentioned this pull request Jun 20, 2019

v2: process is not defined error when bundled with webpack #412

Closed

Tune build scripts to properly check for existence of process object

d16a77e

th0r changed the title ~~Check for existence of process object~~ Check for existence of process object (fix for v2) Jul 4, 2019

mcollina changed the base branch from master to v2.x July 4, 2019 13:11

th0r mentioned this pull request Jul 8, 2019

Check for existence of process object (fix for v3) #414

Closed

rsbondi reviewed Sep 13, 2019

View reviewed changes

mcollina closed this May 23, 2022

Check for existence of process object (fix for v2) #413

Check for existence of process object (fix for v2) #413

Conversation

th0r commented Jun 20, 2019 • edited Loading

th0r commented Jun 24, 2019 • edited Loading

mcollina commented Jun 29, 2019

th0r commented Jul 1, 2019

vweevers commented Jul 1, 2019 • edited Loading

th0r commented Jul 1, 2019

vweevers commented Jul 1, 2019

mcollina commented Jul 1, 2019

vweevers commented Jul 1, 2019

th0r commented Jul 1, 2019 • edited Loading

th0r commented Jul 1, 2019 • edited Loading

vweevers commented Jul 1, 2019 • edited Loading

th0r commented Jul 1, 2019

th0r commented Jul 2, 2019

mcollina commented Jul 4, 2019

th0r commented Jul 4, 2019

mcollina commented Jul 4, 2019

th0r commented Jul 4, 2019

mcollina commented Jul 4, 2019

th0r commented Jul 4, 2019 • edited Loading

th0r commented Jul 4, 2019 • edited Loading

vweevers commented Jul 5, 2019

th0r commented Jul 5, 2019 • edited Loading

vweevers commented Jul 5, 2019

th0r commented Jul 5, 2019 • edited Loading

vweevers commented Jul 5, 2019

th0r commented Jul 5, 2019 • edited Loading

mcollina commented Jul 5, 2019

th0r commented Jul 5, 2019

mcollina commented Jul 5, 2019

th0r commented Jul 5, 2019 • edited Loading

th0r commented Jul 5, 2019 • edited Loading

vweevers commented Jul 5, 2019

mcollina commented Jul 5, 2019

th0r commented Jul 5, 2019

mcollina commented Jul 5, 2019

th0r commented Jul 5, 2019

mcollina commented Jul 5, 2019

th0r commented Jul 5, 2019

mcollina commented Jul 5, 2019

th0r commented Jul 5, 2019 • edited Loading

mcollina commented Jul 5, 2019

th0r commented Jul 5, 2019

mcollina commented Jul 5, 2019

th0r commented Jul 8, 2019

th0r commented Jul 22, 2019 • edited Loading

mcollina commented Jul 22, 2019

th0r commented Aug 2, 2019

kostos commented Aug 6, 2019

rsbondi Sep 13, 2019

Choose a reason for hiding this comment

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

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

th0r commented Jun 20, 2019 •

edited

Loading

th0r commented Jun 24, 2019 •

edited

Loading

vweevers commented Jul 1, 2019 •

edited

Loading

th0r commented Jul 1, 2019 •

edited

Loading

th0r commented Jul 1, 2019 •

edited

Loading

vweevers commented Jul 1, 2019 •

edited

Loading

th0r commented Jul 4, 2019 •

edited

Loading

th0r commented Jul 4, 2019 •

edited

Loading

th0r commented Jul 5, 2019 •

edited

Loading

th0r commented Jul 5, 2019 •

edited

Loading

th0r commented Jul 5, 2019 •

edited

Loading

th0r commented Jul 5, 2019 •

edited

Loading

th0r commented Jul 5, 2019 •

edited

Loading

th0r commented Jul 5, 2019 •

edited

Loading

th0r commented Jul 22, 2019 •

edited

Loading