Shim modules that expect `this` instead of `window`

[tylersticka]

The browserify-shim package is awesome, and 99% of the time it works great (once I'm sure to read the docs and configure things correctly, of course)! That said, there's an issue I've encountered on a couple of projects and, having scoured the list of issues as well as Google and StackOverflow search results, I figured it might make sense to go straight to the source and ask for advice. :)

Basically, the shim seems to work beautifully whenever a script expects globals or window as arguments:

(function (w, $) {
  // etc...
}( ))(window, jQuery);

But when a script expects this to be window, things get bumpier:

(function (w, $) {
  // etc...
}( ))(this, this.jQuery);

The script will be included just fine, but in-browser it will log errors about w and $ being undefined whenever it attempts to access them.

Here's a real-world script I had some trouble shimming, but I've encountered a handful of them: https://github.com/filamentgroup/fixed-fixed

In the past I've resolved this issue by either creating local versions of the dependencies with this substituted for window, changed my build process so all dependencies are concatenated ahead of my core app.js, or switching to some other dependency-management solution entirely. Any advice would be appreciated! Thanks in advance.

[thlorenz]

Interesting case, this already breaks just due to browserify wrapping the code in a function which will change what this is. I suppose these libs expect this to be the window?

Could you submit a slimmed down repro case so I could use it as a fixture for a test?
I'm not sure if its possible, but I'd like to make an attempt in finding some trick to make even these libs work.

One way I'm thinking of is to rewrite the code automatically to change the this in the outermost self invoking function to window.

That being said, libs that do the above are inherently broken since they assume to always run on the script level where this is the window which is never guaranteed.
So in parallel to the efforts here to work around this you should try to convince the authors to fix that.

[tylersticka]

Could you submit a slimmed down repro case so I could use it as a fixture for a test?

Certainly! Here's an example module:

// Non-CommonJS module that expects `this` to be `window`
;(function (w) {
  if (w.document === undefined) {
    console.log("this isn't window :(");
  } else {
    console.log("this is window :)");
  }
})(this);

With this configuration in package.json:

"browserify-shim": {
  "./module": { "exports": null }
}

Result: When the module is required by a script that is bundled, the resulting bundle will log this isn't window :( in the console when included in a page.

Let me know if that's adequate or not.

That being said, libs that do the above are inherently broken since they assume to always run on the script level where this is the window which is never guaranteed.

A colleague of mine suggested that this may be a side-effect of JavaScript linting/hinting tools that discourage the use of window. It wouldn't be obvious to me that these tools often have browser or global flags that are off by default. It seems surprisingly common. 😕

[kjvalencik]

A simple fix would be to wrap shimmed files in a function expression before bundling them.

[thlorenz]

@kjvalencik not sure what you mean, they are already wrapped - have a look at the generated bundle.

[kjvalencik]

@thlorenz, you are calling the function with the current this which is the browserify bundle. If you invoke the function directly, without call, it will be invoked in the global context and this will point to window in a browser.

E.g.,

(function (global){
    // this === window
}(typeof self !== "undefined" ? self : typeof window !== "undefined" ? window : {}));

[thlorenz]

@kjvalencik interesting idea, but I'm not sure if this is 100% correct.

The wrapper that browserify-shim creates is inside the browserify function wrapper. So in this case this would be whatever browserify passes as this. Overriding it with the global is done in order to provide the this that some older libs expect here since they assume to run in the global context.

One option would be to be smarter about the global we pass as this although I don't agree with your sample (where does self come from?). In addition you are ignoring the global object which could break other things.

However if you figure out a change that fixes the above problem and still passes all existing tests, I'd be happy to merge it.

[kjvalencik]

@thlorenz, Nevermind, I was looking at the wrong file. I tried to reproduce @tylersticka's issue and was unable to with either the simple example or FixedFixed.

[thlorenz]

@kjvalencik so the repro case above doesn't repro for you?

@tylersticka could you please push your repro case to github and add a package.json such that we can reliably repro your case via:

git clone https://github.com/you/your-repo && cd your-repo
npm test

Thanks.

[tylersticka]

@thlorenz No prob! I'm probably doing this wrong, but here's my best foot forward: https://github.com/tylersticka/browserify-shim-window-global-issue

I had a coworker follow the README directions and they were able to reproduce the issue. But I'd love to find out that we're both doing something wrong! That'd be the easiest solution ever. 😃

[thlorenz]

Ok having a look as soon as I have some extra time on my hands, unless @kjvalencik beats me to it and fixes your example ;)

[kjvalencik]

@tylersticka The issue is that the path to module.js is incorrect and it was causing it to not be shimmed at all. You can see this by looking at the bundle.js and seeing that there isn't a closure around the module redefining this and the require is not being replace with a reference to window.

Below fixes the issue in your example:

https://github.com/tylersticka/browserify-shim-window-global-issue/pull/1/files

[tylersticka]

@kjvalencik You're right, thanks!

If you don't mind, I'm going to try one more thing to mimic exactly how I saw this occurring in my more complicated project. I hope I'm just a dummy and got the paths wrong there, too, but I also want to make sure I didn't just dumb my example down too much, eliminating the point of failure. I'll post again with either an updated test case or confirmation that "yep, I'm a dummy." Thanks for your help!

[kjvalencik]

@tylersticka If your project is on public github, I can take a peek at that instead of trying to create something simpler.

[tylersticka]

@kjvalencik Thank you for offering, but unfortunately I only experienced this issue in client work I'm not at liberty to share publicly!

So, I just tried to mimic our production setup a bit more closely by running Browserify through an index.js file (instead of via the CLI) and by including one of the actual modules that gave us trouble before, and with @kjvalencik's example I did not encounter this issue again.

Looking back at that production code and comparing it to my simpler test case, I think my key takeaway is that, when in doubt, I should be verbose in how I specify paths. So, if the following isn't working...

"browserify-shim": {
  "some-module": { "exports": null }
}

I should instead try:

"browserify-shim": {
  "./path/to/some-module.js": { "exports": null }
}

Or alternatively:

"browser": {
  "some-module": "./path/to/some-module.js"
},
"browserify-shim": {
  "some-module": { "exports": null }
}

For some reason, even after reading and re-reading the documentation it just didn't "click" for me that this would solve my problem.

Thank you both for your help!

[thlorenz]

@tylersticka if you have suggestions in how to improve the documentation, please lmk.

I felt this section and this one outlined both alternatives pretty clearly.Even the example at the very top of the readme has a browser field.

However I'm not sure if I'm missing something, What may be obvious to me (the author) may not be so to others. So any suggestions are appreciated.

Closing this issue though, but feel free to open another one or even better a PR regarding documentation improvements.Alternatively you could contribute to browserify-shim receipes wiki.

[tylersticka]

@thlorenz Quick question that will help me provide feedback on the documentation: Does browserify-shim have knowledge of npm-installed modules?

So for example, if I were to do npm install --save-dev some-module, with this configuration:

"browserify-shim": {
  "some-module": { "exports": null }
}

...would it know to shim when I call require('some-module'), or will require('some-module') include the un-shimmed module unless I specify the alias and full path?

In the real-world example of this issue that I encountered, we were including a module via npm and configuring it basically as I just described. Specifying the full path instead of referring to the package name resolves the issue. So the part that wasn't quite obvious to me was that I needed to specify a full path.

But if that's not the case, it's likely specific to our unique project setup or this particular module, in which case it's difficult for me to provide specific feedback.

[thlorenz]

@tylersticka not sure why you would shim npm installed modules. Those should work with browserify out of the box. But if you were to do so, browserify will find them and pass them to browserify-shim, so it would work.

[thlorenz]

Actually not 100% sure if this would work without the browser field since browserify-shim will look there, but as far as I remember it'll fall back to let browserify figure it out if it's not found.

"browserify-shim": {
  "some-module": { "exports": null }
}

You could easily write a test case to see if it works.
Use diagnostics to see what it does.

[tylersticka]

Thanks, @thlorenz, but I think my issue is resolved for now, and it's difficult to come up with a more appropriate test-case until I encounter this again in a production environment. I'll be sure to follow up if that happens, but I'm afraid I might be chasing shadows at this point!

Thank you and @kjvalencik for your help, I really appreciate it!

[thlorenz]

Ok thanks @tylersticka.
If you run into another problem feel free to open another issue ;)