thlorenz / browserify-shim

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

Docs referring to `window` are a little confusing #210

Closed YPCrumble closed 8 years ago

YPCrumble commented 8 years ago

I really like this library and try to answer Stack Overflow questions from time to time. This is an example of a really common problem: http://stackoverflow.com/questions/36793438/browserify-shim-doesnt-seem-to-attach-tether-to-window-object

What do the docs mean when they say the following?

x exports window.$

Most people think that if they add <script srct="/path/to/bundle.js"> that $ will get exposed on the window...which isn't the case unless you manually add window.$ to your bundle, or the equivalent (see a previous issue I'd brought up and resolved, #188 ).

I'd love to try to contribute by clarifying the documentation but want to make sure I understand what is actually going on before attempting that.

bendrucker commented 8 years ago

That one particular case doesn't seem to be handled correctly. We'd definitely take a PR to fix it. In general we try to keep browserify-shim stable and usable as a shim but aren't undertaking new features directly without a substantial PR.

bendrucker commented 8 years ago

We'd welcome a PR about that behavior in lieu of an actual code change btw!

YPCrumble commented 8 years ago

Sorry - not sure I follow. Are you saying that in the example from the docs, x really should be available on the window object and you'd like that to be the way the code works/this is a bug to fix?

Or are you saying that you'd want the documentation to clarify how to expose a variable on the window object using window.$ for example? This would also include clarifying that the example I referenced in the docs above wouldn't expose x on the browser's window object. I guess I'm still confused what x exports window.$ actually intends to convey if it isn't actually adding x to the browser's window :).

bendrucker commented 8 years ago

Yeah the docs say:

Modules that just declare a var foo = ... on the script level and assume it gets attached to the window object. Since the only way they will ever be run is in the global context — "ahem, … NO?!"

That doesn't seem to be the case. Maybe it was a regression at some point, not sure if there's a test.

Ideally I'd like to review a PR to get that fixed if you're able to identify the necessary cases and even the source code changes. I'd be more than happy to make some tests if you're able to get the ball rolling.

In lieu of actually solving this bug, I'd be ok explicitly telling people to expose global var statements manually once in their code. A good way to do this might just be another shim file that's an entry point before your code, e.g. browserify -e shim.js index.js.

YPCrumble commented 8 years ago

Awesome! I'd love to work on the PR. I'll say that I'm not an expert coder so I might have some questions along the way but will see what I can do :).

bendrucker commented 8 years ago

For sure! Happy to help w/ wherever you start. A pull request is just an issue with code. The extra effort comes with a lot of extra value to the project, even if it's not the complete solution. I'm going to try to make time today to write a failing case for #188.