Skip to content
Permalink
Browse files
Register as a CommonJS async module if in that kind of environment. F… 
…ixes #7102.
  • Loading branch information
@jrburke @csnover
jrburke authored and csnover committed Dec 27, 2010
1 parent 01cba2e commit 6ffa730
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 23 deletions.
5 src/core.js
View file
@@ -886,6 +886,11 @@ function doScrollCheck() {
jQuery.ready();
}

// Expose jQuery as an Asynchronous Module
if ( typeof define !== "undefined" ) {

This comment has been minimized.

Sign in to view

Copy link
@chicheng

chicheng Dec 28, 2010

Write it in a separate file and exclude it from normal build?

This comment has been minimized.

Sign in to view

Copy link
@jrburke

jrburke Dec 28, 2010

Author Contributor

Disclaimer, these are my personal thoughts, I do not speak for the jQuery team: I believe the path of generally not supplying different builds of jQuery with different feature sets is a great approach to keep things simple for developers: no matter what jQuery file they use, as long as it is the same version, the same set of capabilities is always available. So providing this capability as an optional build I believe will confuse things.

So, why include this change at all? The goal of this kind of change is to allow moving away from using a global to define jQuery, for environments that support that type of change. Modular, non-global-based development is more scalable for the future, and it is most effective for CDN-type of deployments, and for allowing on-demand loading of jQuery itself without conflicting with other jQuery versions in the page. The noConflict approach has edge cases that fail for dynamically loaded scripts that depend on a specific version of jQuery. The define() API is supported by more than one script loader implementation, so it is a good choice for a module API to support.

This comment has been minimized.

Sign in to view

Copy link
@jaubourg

jaubourg Dec 28, 2010

Member

Not really a fan of this commit. Apart from the fact it makes massive assumptions about what define actually is (and may have unforseeable consequences in existing apps), how is it supposed to avoid conflicts? If I load say 1.5.0 then 1.5.1, wouldn't "jquery" be defined as the last one loaded? I fail to see how it can be better (or worse) than noConflict (seeing as nothing else has changed, so jQuery is still defined globally and attached to windows)?

Shouldn't we at least define a "jquery-{version}" too?

The more I think about it, the more hasty it seems to me. Really, I fail to see the gain (in anything but the most minimal of environments) but I clearly see how it can break while solving nothing in anything else. I'd much rather see a more ambitious patch that actually tackles the whole problem.

This comment has been minimized.

Sign in to view

Copy link
@dmethvin

dmethvin Dec 28, 2010

Member

If this "standard" truly has not been nailed down yet as khs4473 says, then I'd recommend backing this out.

This comment has been minimized.

Sign in to view

Copy link
@SlexAxton

SlexAxton Dec 28, 2010

Member

Regardless of the CommonJS standard, they are not a real standards organization, and waiting for them to come to a conclusion on this seems like something that will take forever/never happen. As someone who loads jQuery in as a module and has to clean up after the noConflict issues, I think this type of thing is relatively safe. Perhaps there's a way we could test for correct functionality or something? I don't really know, but can anyone find any current implementations of define that don't meet this pattern?

To Jaubourg, it could end up overriding the global jQuery name, or whatever, but the idea is that you pull it in as a module. Just like with any other module, defining two versions of that module in the same context will override the initial module from that point on, that's to be expected. It's not a replacement for noConflict it's a module definition. Though, this would would mean we should add code to noConflict to potentially replace the module after it's overriden etc. to make it consistent.

Idk. I like the idea. I think we need something like this in here. Perhaps just a few more checks?

This comment has been minimized.

Sign in to view

Copy link
@dmethvin

dmethvin Dec 28, 2010

Member

Well if we support it internally, we are blessing this current interface for better or worse. If we don't do this commit, is it impossible to use jQuery with CommonJS?

This comment has been minimized.

Sign in to view

Copy link
@jrburke

jrburke Dec 28, 2010

Author Contributor

As to the questions about standards and what jQuery should try to support, I wrote a blog post to make the case for this kind of commit.

This comment has been minimized.

Sign in to view

Copy link
@jrburke

jrburke Dec 28, 2010

Author Contributor

I left off a function check for define since it seemed unlikely to make a difference in avoiding conflicts and I wanted to limit the size of the patch. The chance that define is an object but not a function seems smaller than if define is just an incompatible function that is already declared. Note that any API name is likely to have the possibility of conflicts, but they are all solvable by making sure jQuery is loaded first in the page (which it normally is). However, I can do a patch that adds a function check if that is desirable.

This comment has been minimized.

Sign in to view

Copy link
@SlexAxton

SlexAxton Dec 28, 2010

Member

As far as size goes, I think it may be smaller with the function check

if ( typeof define !== "undefined" ) {

vs.

if ($.isFunction(define)) {

Technically slower, but that's hardly a worry here.

This comment has been minimized.

Sign in to view

Copy link
@SlexAxton

SlexAxton Dec 28, 2010

Member

kitgoncharov: you caught me.

Also: snover did a search on google code (which admittedly is not close to comprehensive), but there was only one app that would have an issue from what we could tell out of all the used a define variable. Most are local or own-properties.

http://www.google.com/codesearch?hl=en&lr=&q=\bdefine\s%2B%3D+lang%3Ajavascript&sbtn=Search

or

http://www.google.com/codesearch?hl=en&lr=&q=%28var\s%2B|window\.%29define\b+lang%3Ajavascript&sbtn=Search

This comment has been minimized.

Sign in to view

Copy link
@codenothing

codenothing Dec 29, 2010

sidechat: wouldn't "jQuery.isFunction(window.define)" be smaller after minification?

This comment has been minimized.

Sign in to view

Copy link
@jrburke

jrburke Dec 29, 2010

Author Contributor

I think typeof define === 'function' is a great, legitimate alternative for the check, and improves the expectations on define. I prefer to use that over referencing "window" in the check, best to keep as many JS environment assumptions out of the test, in case this code ever executes in a non-browser environment. It may be unlikely for jQuery but still a nice goal to keep in mind.
define( "jquery", [], function () { return jQuery; } );
}

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

7 test/data/testinit.js
View file
@@ -1,7 +1,12 @@
var jQuery = this.jQuery || "jQuery", // For testing .noConflict()
$ = this.$ || "$",
originaljQuery = jQuery,
original$ = $;
original$ = $,
commonJSDefined;

function define(module, dependencies, callback) {
commonJSDefined = callback();
}

/**
* Returns an array of elements with the given IDs, eg.
46 test/unit/core.js
View file
@@ -12,7 +12,9 @@ test("Basic requirements", function() {
});

test("jQuery()", function() {
expect(23);
expect(24);

strictEqual( commonJSDefined, jQuery, "CommonJS registered (Bug #7102)" );

// Basic constructor's behavior

@@ -151,7 +153,7 @@ test("selector state", function() {
test = jQuery("#main").eq(0);
equals( test.selector, "#main.slice(0,1)", "#main eq Selector" );
equals( test.context, document, "#main eq Context" );

var d = "<div />";
equals(
jQuery(d).appendTo(jQuery(d)).selector,
@@ -253,38 +255,38 @@ test("isPlainObject", function() {

// The use case that we want to match
ok(jQuery.isPlainObject({}), "{}");

// Not objects shouldn't be matched
ok(!jQuery.isPlainObject(""), "string");
ok(!jQuery.isPlainObject(0) && !jQuery.isPlainObject(1), "number");
ok(!jQuery.isPlainObject(true) && !jQuery.isPlainObject(false), "boolean");
ok(!jQuery.isPlainObject(null), "null");
ok(!jQuery.isPlainObject(undefined), "undefined");

// Arrays shouldn't be matched
ok(!jQuery.isPlainObject([]), "array");

// Instantiated objects shouldn't be matched
ok(!jQuery.isPlainObject(new Date), "new Date");

var fn = function(){};

// Functions shouldn't be matched
ok(!jQuery.isPlainObject(fn), "fn");

// Again, instantiated objects shouldn't be matched
ok(!jQuery.isPlainObject(new fn), "new fn (no methods)");

// Makes the function a little more realistic
// (and harder to detect, incidentally)
fn.prototype = {someMethod: function(){}};

// Again, instantiated objects shouldn't be matched
ok(!jQuery.isPlainObject(new fn), "new fn");

// DOM Element
ok(!jQuery.isPlainObject(document.createElement("div")), "DOM Element");

// Window
ok(!jQuery.isPlainObject(window), "window");

@@ -298,7 +300,7 @@ test("isPlainObject", function() {
document.body.removeChild( iframe );
start();
};

var doc = iframe.contentDocument || iframe.contentWindow.document;
doc.open();
doc.write("<body onload='window.parent.iframeDone(Object);'>");
@@ -659,7 +661,7 @@ test("jQuery.merge()", function() {

// Fixed at [5998], #3641
same( parse([-2,-1], [0,1,2]), [-2,-1,0,1,2], "Second array including a zero (falsy)");

// After fixing #5527
same( parse([], [null, undefined]), [null, undefined], "Second array including null and undefined values");
same( parse({length:0}, [1,2]), {length:2, 0:1, 1:2}, "First array like");
@@ -694,7 +696,7 @@ test("jQuery.extend(Object, Object)", function() {
equals( deep1.foo2, document, "Make sure that a deep clone was not attempted on the document" );

ok( jQuery.extend(true, {}, nestedarray).arr !== arr, "Deep extend of object must clone child array" );

// #5991
ok( jQuery.isArray( jQuery.extend(true, { arr: {} }, nestedarray).arr ), "Cloned array heve to be an Array" );
ok( jQuery.isPlainObject( jQuery.extend(true, { arr: arr }, { arr: {} }).arr ), "Cloned object heve to be an plain object" );
@@ -715,13 +717,13 @@ test("jQuery.extend(Object, Object)", function() {
empty = {};
jQuery.extend(true, empty, optionsWithCustomObject);
ok( empty.foo && empty.foo.date === customObject, "Custom objects copy correctly (no methods)" );

// Makes the class a little more realistic
myKlass.prototype = { someMethod: function(){} };
empty = {};
jQuery.extend(true, empty, optionsWithCustomObject);
ok( empty.foo && empty.foo.date === customObject, "Custom objects copy correctly" );

var ret = jQuery.extend(true, { foo: 4 }, { foo: new Number(5) } );
ok( ret.foo == 5, "Wrapped numbers copy correctly" );

@@ -849,10 +851,10 @@ test("jQuery.makeArray", function(){

test("jQuery.isEmptyObject", function(){
expect(2);

equals(true, jQuery.isEmptyObject({}), "isEmptyObject on empty object literal" );
equals(false, jQuery.isEmptyObject({a:1}), "isEmptyObject on non-empty object literal" );

// What about this ?
// equals(true, jQuery.isEmptyObject(null), "isEmptyObject on null" );
});
@@ -878,23 +880,23 @@ test("jQuery.proxy", function(){

test("jQuery.parseJSON", function(){
expect(8);

equals( jQuery.parseJSON(), null, "Nothing in, null out." );
equals( jQuery.parseJSON( null ), null, "Nothing in, null out." );
equals( jQuery.parseJSON( "" ), null, "Nothing in, null out." );

same( jQuery.parseJSON("{}"), {}, "Plain object parsing." );
same( jQuery.parseJSON('{"test":1}'), {"test":1}, "Plain object parsing." );

same( jQuery.parseJSON('\n{"test":1}'), {"test":1}, "Make sure leading whitespaces are handled." );

try {
jQuery.parseJSON("{a:1}");
ok( false, "Test malformed JSON string." );
} catch( e ) {
ok( true, "Test malformed JSON string." );
}

try {
jQuery.parseJSON("{'a':1}");
ok( false, "Test malformed JSON string." );

0 comments on commit 6ffa730

Please sign in to comment.