Fix detection of root object

[targos]

self is compatible with Window and Web Workers (see this article).
Checking for this is breaking browserify builds that run in Workers.

Another solution is to simply do var root = self;. What is the reason to have a fallback to this ?

[defunctzombie]

has this been tested in non-worker environment?

[targos]

Yes, I'm using it in usual window environment as well.
CI seems to be run in browsers and passed without any error : https://travis-ci.org/visionmedia/superagent/jobs/65554746

[oleics]

The PR will not work for code in browserify-bundles, as somehow self is not window inside a module.

Following should cover pretty much all environments:

var root;
if(typeof window !== 'undefined') root = window;      // Browser
else if(typeof self !== 'undefined') root = self;     // Web Worker
else if(typeof global !== 'undefined') root = global; // Web Worker, but in a module (browserify)
else root = this;                                     // everything else

[oleics]

Just checked the issue again:
The reason that using superagent in browserify-bundles running in web-worker-env fails, is not that self is undefined. Instead, this is defined. So, this does the trick:

var root;
if(typeof window !== 'undefined') root = window;      // Browser
else if(typeof self !== 'undefined') root = self;     // Web Worker
else root = this;                                     // everything else

Could be rewritten as a one-liner, but I would not do so for readability.

[targos]

@oleics I cannot reproduce a situation where self is not equal to window with browserify. When does that happen ?

[oleics]

@targos

In web-worker-env, self is not window, as code running in a web-worker does not have access to window. self in web-workers is - simply put - a new context. See https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Using_web_workers

But that is not the problem here. Let me refer to this:

var root = 'undefined' == typeof self
  ? this
  : self;

this might not always be window, especially when it comes to module-systems like nodejs/browserify or whatever-module-system. It is unsave to rely on this beeing always window. I think it is better to be explicit here and test for window first, then self, and then (as the last resort) this.

[targos]

@oleics
I agree with your explanation, and my PR is here exactly to fix the fact that this is defined in browserify builds but not with the expected value.
I can add a check for window first if that feels safer, but it should not be needed because self is a reference to window in the browser and to the worker global scope in web workers (see link in the OP).

[oleics]

@targos
The check for self alone makes me a little nervous, as we all do var self = this. But this should really only be the last resort. I would throw an error if neither window nor self is defined.

[targos]

I pushed a change to safely check for window first, PTAL

[oleics]

@targos Thanks.
@defunctzombie What do you think, merge?

[defunctzombie]

please follow existing coding style (use braces)

What is the consensus around the changeset now? Is this is correct version?

[targos]

@defunctzombie braces added.
This is a correct, more conservative version than the first one. I think both behave exactly the same, but this one has the advantage to be explicit about about browser and web worker compatibility.

[taish]

👍 👍
I am facing same problem using webpack & webworker.

[mohsen1]

+1
Please merge this. This module needs Web Worker support. It's deep in my dependency chain, I can't just use a fork.
Thanks!

[oleics]

thanks a lot, just in time.