Skip to content

Commit

Permalink
Landing pull request 586. Create exports.js for exporting jQuery to w…
Browse files Browse the repository at this point in the history
…indow and AMD. Fixes #10687.

More Details:
 - #586
 - http://bugs.jquery.com/ticket/10687
  • Loading branch information
jrburke authored and timmywil committed Nov 14, 2011
1 parent 499d7e4 commit 8bc60ba
Show file tree
Hide file tree
Showing 7 changed files with 35 additions and 24 deletions.
3 changes: 2 additions & 1 deletion Makefile
Expand Up @@ -27,7 +27,8 @@ BASE_FILES = ${SRC_DIR}/core.js\
${SRC_DIR}/ajax/xhr.js\
${SRC_DIR}/effects.js\
${SRC_DIR}/offset.js\
${SRC_DIR}/dimensions.js
${SRC_DIR}/dimensions.js\
${SRC_DIR}/exports.js

MODULES = ${SRC_DIR}/intro.js\
${BASE_FILES}\
Expand Down
14 changes: 0 additions & 14 deletions src/core.js
Expand Up @@ -934,20 +934,6 @@ function doScrollCheck() {
jQuery.ready();
}

// Expose jQuery as an AMD module, but only for AMD loaders that
// understand the issues with loading multiple versions of jQuery
// in a page that all might call define(). The loader will indicate
// they have special allowances for multiple jQuery versions by
// specifying define.amd.jQuery = true. Register as a named module,
// since jQuery can be concatenated with other files that may use define,
// but not use a proper concatenation script that understands anonymous
// AMD modules. A named AMD is safest and most robust way to register.
// Lowercase jquery is used because AMD module names are derived from
// file names, and jQuery is normally delivered in a lowercase file name.
if ( typeof define === "function" && define.amd && define.amd.jQuery ) {
define( "jquery", [], function () { return jQuery; } );
}

return jQuery;

})();
22 changes: 22 additions & 0 deletions src/exports.js
@@ -0,0 +1,22 @@
(function( jQuery ) {

// Expose jQuery to the global object
window.jQuery = window.$ = jQuery;

// Expose jQuery as an AMD module, but only for AMD loaders that
// understand the issues with loading multiple versions of jQuery
// in a page that all might call define(). The loader will indicate
// they have special allowances for multiple jQuery versions by
// specifying define.amd.jQuery = true. Register as a named module,
// since jQuery can be concatenated with other files that may use define,
// but not use a proper concatenation script that understands anonymous
// AMD modules. A named AMD is safest and most robust way to register.
// Lowercase jquery is used because AMD module names are derived from
// file names, and jQuery is normally delivered in a lowercase file name.
// Do this after creating the global so that if an AMD module wants to call
// noConflict to hide this version of jQuery, it will work.
if ( typeof define === "function" && define.amd && define.amd.jQuery ) {

This comment has been minimized.

Copy link
@jdalton

jdalton Nov 15, 2011

Member

I'm curious why this behavior is just handled for jQuery via define.amd.jQuery.
I know lots of libs that allow multiple versions on a page through some form of noConflict() method.

This comment has been minimized.

Copy link
@jrburke

jrburke Nov 16, 2011

Author Contributor

I put a page up on the AMD wiki to explain it.

This comment has been minimized.

Copy link
@davidmurdoch

davidmurdoch Nov 29, 2011

typeof fn lies in certain circumstances; I can't find the details on it now, but @gnarf knows more.

Also, replacing typeof define === "function" with jQuery.isFunction(window.define) saves 2 bytes when deflated with this deflator.

Additionally, changing jQuery's intro to (function( window, define, undefined ) { and outro to })( window, define ); sheds 57 bytes (deflated) from the original when using jQuery.isFunction(define) in place of typeof define === "function".

Note that I'm using http://code.jquery.com/jquery-git.js minified with http://marijnhaverbeke.nl/uglifyjs for the deflated size comparisons...which means the copyright info was removed - which might affect the results of the deflate algo.

This comment has been minimized.

Copy link
@jdalton

jdalton Nov 29, 2011

Member

@davodurdoch Some versions of Chrome, FF, and Safari will return typeof function on regexps because they are callable.

This comment has been minimized.

Copy link
@rwaldron

rwaldron Nov 29, 2011

Member

To back @jdalton's comment, take a look at this: http://es5.github.com/#x11.4.3

This comment has been minimized.

Copy link
@davidmurdoch

davidmurdoch Nov 29, 2011

Any thoughts on slapping define into intro.js and outro.js?

This comment has been minimized.

Copy link
@mikesherov

mikesherov Nov 29, 2011

Member

@davidmurdoch, it was move into it's own file so it could be tested in the unit testing environment, which doesn't include intro.js and outro.js

This comment has been minimized.

Copy link
@davidmurdoch

davidmurdoch Nov 29, 2011

@mikesherov, that's not what I'm asking about. Putting define in intro.js and outro.js allows the JS compressor to shrink any occurrences of define to one character, making jQuery smaller; just like it does with window and undefined (I know undefined is really there for a different reason - but it still reaps the same benefit).

Putting define in the intro/outro also avoids having to use window.define in jQuery.isFunction if the typeof define === "function" line is going to be replaced with that.

This comment has been minimized.

Copy link
@jrburke

jrburke Nov 29, 2011

Author Contributor

I would prefer to keep the check as-is, and I believe comparing minified+gzipped size is more important than just minified size, but I leave it to the jQuery committers to comment.

As the check stands now, it is a standard check for AMD, and using that standard check allows AMD optimizer tools to namespace define to another object and do some file rewriting of jquery.js to use that namespaced define over the global define.

If the jQuery committers feel the changes are important though, then we can add another special case to the namespace rewriting tools. However, I do not believe the typeof check is a problem given the additional .amd check and I do not believe the minification tricks will buy much once gzipped.

This comment has been minimized.

Copy link
@jdalton

jdalton Nov 29, 2011

Member

@jrburke Why not just make a better build tool. You could expose the regexp (which allows customization) used to search for define. Smth like builder.reDefine.

This comment has been minimized.

Copy link
@davidmurdoch

davidmurdoch Nov 29, 2011

All my file-size comparisons were against the deflated versions of the minified source.

When content is "gzipped" it means it has been compressed via DEFLATE and wrapped with GZIP's header and footer. So, when I say "sheds 57 bytes (deflated)" it means "sheds 57 bytes (gzipped, sans header and footer)".

@jrburke, do the AMD optimizer tools tokenize/parse the JS to do the file-rewriting or is it some sort of static string analysis? Or some other method?

This comment has been minimized.

Copy link
@jrburke

jrburke Nov 29, 2011

Author Contributor

@jdalton: I mentioned I would change the build tool if the jQuery folks preferred the other code structure. If the user uses jQuery, underscore and sproutcore and they all do it differently, it will be a complex regexp or AST transform for the end developer to construct. I would prefer that the optimizer just take care of it.

There is great value in having common idioms used consistently across libraries because it allows other people to write simpler optimization tools. But I would rather have the library-specific developers comfortable with their define() use. There is a tradeoff, and local library preferences may be more important than keeping with a common idiom.

@davidmurdoch: thanks for cluing me in on the numbers, I got confused.

As for the namepsace analysis, I am using regexps for that part. I use an AST for a different part of the optimization, but it does not really matter in this case if a regexp or an AST Is used, there still needs to be a pattern that is recognizable as the target of a transform. Using jQuery specific methods means having a custom transform that is aware of the jQuery method names and structure.

This comment has been minimized.

Copy link
@jdalton

jdalton Nov 29, 2011

Member

@jrburke good point :)

define( "jquery", [], function () { return jQuery; } );

This comment has been minimized.

Copy link
@jdalton

jdalton Nov 15, 2011

Member

Also I noticed an empty dependencies array [].
Is that needed?

This comment has been minimized.

Copy link
@aslilac

aslilac Nov 15, 2011

No, it isn't. I've tested it and it works fine without it in RequireJS.

This comment has been minimized.

Copy link
@jrburke

jrburke Nov 16, 2011

Author Contributor

Since the first incarnation of this patch was done (back in early 2011?) I have changed requirejs to work correctly even if the array argument is not there. It is allowed in the AMD spec, requirejs was just a bit behind on that one. Also, I just looked, and the compliance tests for amdjs do not have an explicit test for the define('id', function (){}) signature, but they should. I need to do some small updates to the tests, and I will add an explicit test for the non-array version.

While I know curl and I am pretty sure dojo will work without the array, I would feel better removing it after having the explicit test to the amdjs test suite and notifying the amd-implement group of the change.

}

})( jQuery );
3 changes: 1 addition & 2 deletions src/outro.js
@@ -1,3 +1,2 @@
// Expose jQuery to the global object
window.jQuery = window.$ = jQuery;

})( window );
2 changes: 2 additions & 0 deletions test/index.html
Expand Up @@ -28,6 +28,7 @@
<script src="../src/effects.js"></script>
<script src="../src/offset.js"></script>
<script src="../src/dimensions.js"></script>
<script src="../src/exports.js"></script>

<script src="data/versioncheck.js"></script>

Expand All @@ -51,6 +52,7 @@
<script src="unit/effects.js"></script>
<script src="unit/offset.js"></script>
<script src="unit/dimensions.js"></script>
<script src="unit/exports.js"></script>

<script>
// html5shiv, enabling HTML5 elements to be used with jQuery
Expand Down
8 changes: 1 addition & 7 deletions test/unit/core.js
Expand Up @@ -225,12 +225,6 @@ test("browser", function() {
});
}

test("amdModule", function() {
expect(1);

equal( jQuery, amdDefined, "Make sure defined module matches jQuery" );
});

test("noConflict", function() {
expect(7);

Expand Down Expand Up @@ -534,7 +528,7 @@ test("isXMLDoc - HTML", function() {

test("XSS via location.hash", function() {
expect(1);

stop();
jQuery._check9521 = function(x){
ok( x, "script called from #id-like selector with inline handler" );
Expand Down
7 changes: 7 additions & 0 deletions test/unit/exports.js
@@ -0,0 +1,7 @@
module("exports", { teardown: moduleTeardown });

test("amdModule", function() {
expect(1);

equal( jQuery, amdDefined, "Make sure defined module matches jQuery" );
});

0 comments on commit 8bc60ba

Please sign in to comment.