Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect global in strict mode and WebWorkers #2153

Merged
merged 1 commit into from
May 14, 2015

Conversation

megawac
Copy link
Collaborator

@megawac megawac commented Apr 14, 2015

fixes #2152

@michaelficarra
Copy link
Collaborator

It bothers me that the only detection we do now is via de facto self-references on the global object. I wish there was a more reliable way to do this, though at the same time I don't want it to be possible to get a reference to the global object in strict mode.

// Establish the root object, `window` in the browser, or `exports` on the server.
var root = this;
// Establish the root object, `window` in the browser, or `global` on the server.
var root = typeof self == 'object' && self.Object == Object && self;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.Object == Object would be better as self.self === self.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call

@jridgewell
Copy link
Collaborator

I'm not entirely sure how to test, but what are self and global in ES6 modules? I assume they're either the browser's window or node's global and the only difference is the context of the module is undefined instead of one of those?

@megawac
Copy link
Collaborator Author

megawac commented Apr 16, 2015

Yeah this will be undefined so instead we detect the global scope variable, as you mentioned self in the browser & global in node. I think your fix would work as well root = this || {}

@michaelficarra
Copy link
Collaborator

@megawac no, I believe this will be a ReferenceError.

@jridgewell
Copy link
Collaborator

👍. We should also apply this to Backbone.

@jashkenas
Copy link
Owner

I've just fixed this in Backbone like so (a little looser): jashkenas/backbone@5a6ffcb

Any objections?

megawac referenced this pull request in jashkenas/backbone May 13, 2015
@jridgewell
Copy link
Collaborator

It probably won't hurt much in Backbone, but avoiding self will prevent underscore from working in WebWorkers.

workers run in another global context that is different from the current window. Thus, using the window shortcut to get the current global scope (instead of self) within a Worker will return an error. - MDN

@jridgewell
Copy link
Collaborator

Merging in light of jashkenas/backbone#3602.

jridgewell added a commit that referenced this pull request May 14, 2015
Detect global in strict mode and WebWorkers
@jridgewell jridgewell merged commit fd11c93 into jashkenas:master May 14, 2015
@megawac
Copy link
Collaborator Author

megawac commented May 14, 2015

@jashkenas may justify a patch release

On Thu, May 14, 2015 at 12:02 PM, Justin Ridgewell <notifications@github.com

wrote:

Merged #2153 #2153.


Reply to this email directly or view it on GitHub
#2153 (comment).

@megawac megawac deleted the global branch May 14, 2015 16:13
@zertosh
Copy link
Contributor

zertosh commented May 15, 2015

I would like to point out that if you use a build tool like browserify, a reference to global, causes the module to be wrapped in:

(function (global){
/** your original module **/
}).call(this,typeof global !== "undefined" ? global : typeof self !== "undefined" ? self : typeof window !== "undefined" ? window : {})

I'm only pointing this out for the benefit of those that use browserify and wonder why underscore gets that wrapper. I'm not suggesting to build underscore with browserify, and I'm not suggesting this is a bad change.

@dcocchia
Copy link

This fix was merged with master in May, 2015. Almost 2 years later, can we release a patch?

@Kosta-Github
Copy link

I second the request from @dcocchia ? can you distribute the fix as a new release, please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Function("return this") to get global?
7 participants