brunch / deppack

Extract node modules to browser.
MIT License
4 stars 13 forks source link

Fix global context for non browser environments #43

Closed herenow closed 7 years ago

herenow commented 7 years ago

Problem:

If we ever try to run the bundle generated by this script in an environment like NodeJS, we will get a "window is not defined" error.

The window variable/context may not be present in environments outside of the browser, in this case, the Global Object is referenced by the global variable, and not window.

Solution:

Fall back to the global variable, if window is not present.

herenow commented 7 years ago

Actually, reading the code again, if we do:

var global = global;

global would be undefined, is it possible for us to rename global to globals?

shvaikalesh commented 7 years ago

Hey @herenow, thank you for the PR.

Could you please expand on

global would be undefined, is it possible for us to rename global to globals?

herenow commented 7 years ago

@shvaikalesh

Since in node we already have a variable named global in the previous scope, because of javascript's hoisting, if we try to do var global = global; it will first initialize a new global variable and try to assign this new undefined variable to itself.

Renaming the variable would "fix" this, like we do here:

https://github.com/brunch/commonjs-require-definition/blob/master/require.js#L4

I've taken a further look into deppack's code, and I'm not sure why we need to set global to window, is there still a use for this?

In my case, where I'm trying the require the bundle inside node (for server rendering), I've defined global.window = global so window is defined when I pass through this code.

shvaikalesh commented 7 years ago

Thanks for the detailed explanation.

What about

if (typeof global === 'undefined') {
    window.global = window;
}
herenow commented 7 years ago

Thats the problem :)

Since javascript hoists all variable declarations to the top, this would actually compile to:

var global; // undefined
if (typeof global === 'undefined') {
    global = window;
}

On Aug 2, 2016, at 11:00 AM, Aleksey Shvayka notifications@github.com wrote:

Thanks for the detailed explanation.

What about

if (typeof global === 'undefined') { var global = window; } — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/brunch/deppack/pull/43#issuecomment-236913119, or mute the thread https://github.com/notifications/unsubscribe-auth/ACpRLlLzPsMHCL2R_fvCGQUpl2aUcQM6ks5qb02FgaJpZM4JZFx6.

shvaikalesh commented 7 years ago

Yop, I have updated my answer. I am not extremely happy with window.global, but it seems like the safest approach.

herenow commented 7 years ago

window.global would be work 👍

Maybe we should just leave as is for now, most users when trying to load a bundle generated for the browser on the server should already be mocking the window anyways.

Let's wait and see if more users stumble upon this problem, I'm probably being too picky with this issue :).