search

thlorenz / browserify-shim

📩 Makes CommonJS incompatible files browserifyable.
MIT License
933 stars 87 forks source link

Shimming scripts that assume 'this' is a reference to the window object #154

Closed alexfiedler closed 9 years ago

alexfiedler commented 9 years ago

Hi there, I am attempting to shim respond.js (https://github.com/scottjehl/Respond) However the script is made as a function expression assuming that 'this' will be the window global object. 

(function(window) {
  //assumes window is the global window object
})(this)

I am trying to bundle it with a number of other scripts on my page but when included, window is not the same as the global window object.

Changing 'this' to an explicit 'window' solves this issue but then I would need to continue maintaining the library.

Has there been a bug like this before? Or am I out of luck with browserify for this one?

bendrucker commented 9 years ago

Always a mystery how people decide that's a good idea. I don't want to make any promises but this one may be pretty easy. I'll take a crack at it.

thlorenz commented 9 years ago

@alexfiedler could you please add a failing test to make it easier for us to fix?

You don't need to include the entire Respond library, just the part that reproduces the problem. You can add it here and use the existing tests as a guide how to do that.

Thanks.

I'm actually a bit surprised that this is not already working since browserify-shim will ensure that this will be the window, so a test will help greatly in trouble shooting this.

bendrucker commented 9 years ago

Wrote a test for this that passes in e759162d5a3bf2d90650bd0f28d38e78e1558d34

thlorenz commented 9 years ago

Ok, so not an issue then. That's what I remembered. Could you double check that we don't already have a test for this scenario?

AFAIK the shim-impress tests should cover this scenario, but even then your addition is more obvious and shorter, so if that's the only place it's covered please pull this into master. No need to republish though.

@alexfiedler if the test doesn't cover your use case please add another one that does.

Thanks.

alexfiedler commented 9 years ago

I double checked my code and It was not applying the transform correctly. I apologize for the inconvenience. Thanks you for extremely prompt help!

thlorenz commented 9 years ago

no worries we got an extra test out of it :)