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

Why is window exposed to required modules? #1588

Closed
natevw opened this issue Feb 19, 2014 · 8 comments
Closed

Why is window exposed to required modules? #1588

natevw opened this issue Feb 19, 2014 · 8 comments
Assignees

Comments

@natevw
Copy link

natevw commented Feb 19, 2014

I'm trying to use a module that tests for a CommonJS instead of browser context via (typeof window === 'undefined').

According to https://github.com/rogerwang/node-webkit/wiki/Differences-of-JavaScript-contexts#wiki-determining-thecontext-ofascript this should work fine, but for some reason this was landed a while ago 7fb1314 and now contradicts the documentation.

How should I be testing for CommonJS? I went with the 'window' check because I figured in the browser who knows what globals (exports,module,global) might have gotten randomly set by preceding scripts, whereas it was (other than this!) less likely that a CommonJS module would have random extra globals hanging around.

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@katanacrimson
Copy link

Interesting. What's the properties, etc. of window? Is it -the- webkit window?

@natevw
Copy link
Author

natevw commented Feb 22, 2014

Sure seems to be. The commit message says simply "Expose window object to Node's context"

@katanacrimson
Copy link

You know, I wonder if there's a better way around this.

@rogerwang - why not set a global - say global.context, with either node or webkit (or blink, whatever) to help identify the current exec context?

@natevw
Copy link
Author

natevw commented Feb 23, 2014

A one-off special check is not going to be great in my library, nor I suspect in others, especially since I know of no existing standard here and I'd hate to have to add checks every random different environment like this that comes along. Why not make users opt-in with require('window') or something so that it matches the documentation again?

@jduncanator
Copy link

@natevm That certainly sounds like a good idea, however is it feasible? Also, what happens if you have an actual module called "window" you wish to require? What happens then?

@tomasdev
Copy link

@rogerwang so... continuing #superagent/95 removing 'window' will be a backwards compatibility challenger.

I don't think the proper check frameworks should do is "is it browser or not" but rather a feature detection to know whether UMD/AMD are present as @jashkenas did at jashkenas/backbone@ab5d2eb and @jdalton did at lodash/lodash@e76685f

In node-webkit, as long as we can still assign variables/functions to the front-end (maybe require('window') as @natevm suggested,) it would be plausible to remove window as a global.

@tomasdev
Copy link

@damianb a priori is a placeholder for the webkit's window, a posteriori is it. Basically Node context is initialized before the browser context, AFAIK.

@nwjs-bot
Copy link

This should be working with latest version now.

In 0.13 we changed to an optimized architecture so more features can be supported, see http://nwjs.io/blog/whats-new-in-0.13/ and it's good for keeping up with Chromium upstream -- we released with Node.js v6.0 and new Chromium versions within 1 day after upstream release.

The new version would fixed many issues reported here and we're scrubbing them. This issue is closed as we believe it should be fixed. Please leave a message if it isn't and we'll reopen it.

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

No branches or pull requests

6 participants