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

Request: this binds to the window rather than the exports object. #25

Closed
airportyh opened this issue Nov 2, 2013 · 14 comments
Closed

Comments

@airportyh
Copy link

In the context of a module, currently this binds to the exports object, this is messing up scripts that are using this to mean window. I think make it it bind to window would allow interoperability with more scripts. The change would simply be to change m.export to window on line

https://github.com/substack/browser-pack/blob/master/prelude.js#L31

@mscdex
Copy link

mscdex commented Nov 4, 2013

In node.js modules, this === module.exports. Changing that would break any node modules that currently use this this way.

@esamattis
Copy link
Contributor

@airportyh
Copy link
Author

@mscdex @epeli thanks for the additional info.

I am empathic to this point made by @epeli earlier

On the other hand I've never seen anyone using this to export stuff from node.js modules. People are always using exports or module.exports explicitly. Assigning this to window could be very practical for non-CommonJS libraries, but it wouldn't be the most node.jsish way.

I personally wasn't even aware that this was bound to module.exports in Node, and I am no newb. I just haven't seen this recommended or documented anywhere. Does anyone know of an active module which actually uses this instead of module.exports?

I am currently working on making deamdify work with a larger set of libraries, and as you can imagine, authors who publish in AMD may not be clued in to how Browserify/Node works.

@esamattis
Copy link
Contributor

Remember to think about the worst case too: When you actually encounter a module that uses this to expose stuff and you browserify that module you get very sneaky global leaks...

Personally I'd say no to this request.

@domenic
Copy link

domenic commented Nov 25, 2013

This would break a lot of node code in the browser.

@airportyh
Copy link
Author

Okay. I'll opt for the workaround of wrapping the amd modules with an iife.

@sudodoki
Copy link

Okay, so how should library address window to be compatible with browserify?
Just ran into issue related to this in eligrey/FileSaver.js#57 and would appreciate any insights on how to address the matter in case script to need to access to window variable while being used with browserify.

@ghost
Copy link

ghost commented Dec 27, 2013

@Sudoki Just use window instead of this. I never got why people don't just do that.

@andreypopp
Copy link
Contributor

I wonder if browserify-shim transform can have a configurable option to wrap such "modules" into a function which executes in window's context? /cc @thlorenz

@thlorenz
Copy link
Contributor

@substack some older libs still use this cause they expect to run on script level. It makes it hard to use those with browserify even with browserify-shim. Even though that is not browserifys fault, if there was a way to make this work out of the box that'd be great.

@andreypopp I'd rather find a solution that works for all modules. If you think this can (and should) be solved via browserify-shim, please file an issue (ideally with repro case) over there and we can figure out how to do that.

@airportyh
Copy link
Author

The workaround I used was:

;(function(){
   // original code
}.call(window));

@thlorenz
Copy link
Contributor

Since this is very browser specific, adding it to browserify would break things in node (you can run a bundle in node).

Would this problem be solved if browserify-shim would wrap modules like this? Possibly the issues appear most likely when shimming modules?

@airportyh
Copy link
Author

@thlorenz yeah, that's what I mean, it would be done before being passed to browserify.

@thlorenz
Copy link
Contributor

So we'll fix it in browserify-shim then.

I'd appreciate an issue with a repro case in the browserify-shim issues :)

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

7 participants