Wrapping style breaks AMD inclusion when built into other projects

[ixtli]

(This is a bit of a complicated one so thanks for bearing with me.)

The tl;dr: of this is that as of jQuery UI 1.11 all modules support loading with AMD loaders like RequireJS. Referring to this by the name jQuery breaks the way jQuery conforms to AMD and you should change the last line of your built source to reflect the convention .

The reason for this is because they wrap the entire build jquery-ui.js file in the following:

(function( factory ) {
    if ( typeof define === "function" && define.amd ) {
        // AMD. Register as an anonymous module.
        define([ "jquery" ], factory );
    } else {
        // Browser globals
        factory( jQuery );
    }
}(function( $ ) {
[...]
}));

So, every module loaded has access to the top level jQuery object $ and they all operate on it by doing $.thing(. This project appears to break the convention by referring to jquery by its exported name jQuery here: https://github.com/jquery/jquery-color/blob/master/jquery.color.js#L663

Perhaps there is a reason for this, but it causes problems with AMD loading as I will explain below. The easiest fix is to change jQuery in line 663 to $.

If you're interested in the gorier details (and why I think this works for most people) they are as follows:

I am including jquery-ui with RequireJS. jQueryUI obviously requires jQuery as a dependency. This is achieved by adding the following to your require config:

    shim: {
        'jquery-ui' : { exports: '$', deps: ['jquery'] },
        'jquery': { exports: '$' }
    },

This tells require that it should load jquery before jquery-ui and pass the exported object ($1) as an invocation parameter to jQuery UI. (Hence the }(function( $ ) { in the top snippet.) This allows us to keep jQuery AND jQuery-UI completely out of the global namespace achieving true modularity. The reason I think this works is that the default build of jQuery includes this file: https://github.com/jquery/jquery/blob/master/src/exports/global.js which installs it in (and clobbers) the global namespace as $ and jQuery. However I and other make builds that don't include the global module using the supported custom argument to grunt when making jQuery. ( Reference: https://github.com/jquery/jquery/blob/master/README.md#custom-build-examples ) This produces a build of jquery that does nothing unless executed in the presence of an AMD loader ( reference: https://github.com/jquery/jquery/blob/master/src/exports/amd.js ) for maximum conformance.

Thanks for reading a lot of words about why you should change six characters to one character in your source =)

[scottgonzalez]

Why are you exporting as $ instead of jQuery?

[ixtli]

oh. changing { exports: '$' } to { exports: 'jQuery' } doesn't effect the problem. jquery-ui.js provides $ to the modules it includes and I can't change that.

(Fwiw, exports just defines what global the module exports so that require can load things that wouldn't otherwise play well with AMD. jsdiff for example: https://github.com/kpdecker/jsdiff/blob/master/diff.js . In this case I have a build that doesn't export anything so that line is superfluous. I added it in there to try and be more clear about my point but I could see how it could to the opposite, sorry :/ )

[ixtli]

Also, for reference, the jquery-ui.js exposed to bower is here: https://github.com/components/jqueryui/blob/master/jquery-ui.js#L16

[ixtli]

Perhaps you could do (function(jQuery){ [...] })( jQuery || $ ); ? To test for both? Dunno if that would work on all browsers but its a potential ES5 middle ground.

[scottgonzalez]

The proper fix for this is to add AMD support for jQuery Color and to stop inlining jQuery Color in jQuery UI. Both are already planned.

[scottgonzalez]

Anyway, this isn't a problem with jQuery Color. The problem is in jQuery UI.

[ixtli]

Well sure, if you're going to add AMD support for your bits then that's good enough. I didn't wanna suggest that, though, as it's more work and changing jQuery to $ seemed like a good stopgap. 👍

[scottgonzalez]

Changing jQuery to $ will never be a valid fix. All of our projects only rely on jQuery.

[tjvantoll]

@ixtli I'm confused here and maybe you can help clarify. I tried running this on a new project with jQuery UI 1.11, and a version of jQuery after running grunt custom:-exports/global. I had no issues, and there were no global $ or jQuery variables exposed.

Also note that there is no need to define any shim configuration as of jQuery UI 1.11, as jQuery Core and jQuery UI both support AMD natively.

[ixtli]

@tjvantoll Hey, thanks for your time.

The simplest way to duplicate my issue is like this: http://cl.ly/3S1K332I3W29 . It's a zip with a build of dist/jquery.js after building with grunt custom:-exports/global because there doesn't appear to be a CDN-ized version of it anywhere. It also has a simple index.html that loads require and tries to execute a module that requires in google-cdn's jquery ui 1.11.0. All other resources are loaded from cdnjs and google.

Sorry that it has to be a zip like it's 1997, but I couldn't figure out an easier way to be sure that we're both on the same page with jquery no global.

[ixtli]

To be clear I get 'Uncaught ReferenceError: jQuery is not defined ' on jquery-ui.js:9706 when running that in current chrome.

EDIT: You're right, though. Once I change jquery-ui 1.11 to fix the error I'm getting on line 9706, i can safely remove 'jquery-ui' : { deps: ['jquery'] } which is great =D

[tjvantoll]

@ixtli Ok thanks I see it now. I verified this patch addresses your issue so we'll get that in 1.11.1.

[ixtli]

Yay! Thanks so much. Great response time :D

On 2014/07/14, at 22:39, TJ VanToll notifications@github.com wrote:

@ixtli Ok thanks I see it now. I verified this patch addresses your issue so we'll get that in 1.11.1.

�
Reply to this email directly or view it on GitHub.