Skip to content

Support for node/browserify#55

Merged
le717 merged 4 commits intogabceb:masterfrom
mwiencek:master
Dec 24, 2014
Merged

Support for node/browserify#55
le717 merged 4 commits intogabceb:masterfrom
mwiencek:master

Conversation

@mwiencek
Copy link
Contributor

Fixes two compatibility issues:

  1. Inside wrapped browserify modules, this does not refer to window,
    it's the exports object. Since it doesn't really make sense to use
    this plugin in environments that don't have a window.navigator object,
    the code now just references 'window' directly.
  2. The plugin did a global export if AMD wasn't detected, which assumed
    there was a global jQuery object. The added module.exports-detection
    uses require to pull in jQuery instead.

Fixes two compatibility issues:

  1. Inside wrapped browserify modules, `this` does not refer to `window`,
     it's the `exports` object. Since it doesn't really make sense to use
     this plugin in environments that don't have a window.navigator object,
     the code now just references 'window' directly.

  2. The plugin did a global export if AMD wasn't detected, which assumed
     there was a global jQuery object. The added `module.exports`-detection
     uses `require` to pull in jQuery instead.
@le717
Copy link
Contributor

le717 commented Dec 24, 2014

Have you successfully tested this in AMD and Node/Browerify environments and ensured the tests still pass?

@mwiencek
Copy link
Contributor Author

I've tested the changes in a project using browserify 7.0.1 and jQuery 1.11.2. The tests pass now after 4ad34a3.

require('jquery') won't work out-of-the-box with pre-1.11 jQuery, since they didn't have a main field in package.json, but I assume anyone using older jQuery versions with browserify already have some sort of shim setup for it. And this is still better than not working at all. :)

I don't have any AMD project/setup to test these changes with, but I assume it'll work identical to before because the typeof define === 'function' && define.amd is still the first branch and unchanged, so the only change in the code path for AMD environments should be where it references window directly in the source.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather this written right after the comment at the top (line 15, with a blank line after it and before (function...).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather this written right after the comment at the top (line 15, with a blank line after it and before (function...).

Done so.

@le717
Copy link
Contributor

le717 commented Dec 24, 2014

Just that one minor nit and this should be fine.

@le717 le717 assigned le717 and unassigned le717 Dec 24, 2014
@le717
Copy link
Contributor

le717 commented Dec 24, 2014

Thanks for the contribution!

le717 pushed a commit that referenced this pull request Dec 24, 2014
Support for node/browserify
@le717 le717 merged commit a6a2576 into gabceb:master Dec 24, 2014
@gabceb
Copy link
Owner

gabceb commented Dec 25, 2014

I've added simple RequireJs testing on 3f6c1da

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

Successfully merging this pull request may close these issues.

3 participants