Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

deferred.promise(obj) should work with non-objects. Fixes #12521. Muc…

…h needed unit tests added!
  • Loading branch information...
commit 74cdd784975ba6e628d1934a859faeb017824a5f 1 parent 9c5089a
@jaubourg jaubourg authored
Showing with 19 additions and 2 deletions.
  1. +1 −1  src/deferred.js
  2. +18 −1 test/unit/deferred.js
View
2  src/deferred.js
@@ -44,7 +44,7 @@ jQuery.extend({
// Get a promise for this deferred
// If obj is provided, the promise aspect is added to the object
promise: function( obj ) {
- return typeof obj === "object" ? jQuery.extend( obj, promise ) : promise;
+ return obj != null ? jQuery.extend( obj, promise ) : promise;
@Krinkle
Krinkle added a note

Could do with a comment making it explicit that it is intentional that all objects are supported (even objects that aren't typeof object), e.g. including function objects. Since it was a bugfix and is documented behavior.

Another common test is to verify that something is an object (including function objects) is Object( obj ) !== obj.

@rwaldron Collaborator

@Krinkle The funny thing about this: Object( obj ) !== obj is when you use jshint it will yell about not using new (which is optional), but when you add new it yells about making objects with new Object() vs {} (not optional) :dizzy_face:

@Krinkle
Krinkle added a note

Not anymore it isn't :bowtie:

That's fixed now (jshint/jshint#392) as of JSHint r11 (2012-09-03). The native constructors are allowed to be invoked as functions to trigger the internal methods toNumber, toString, toObject etc. This was already the case for most of them, Object has joined that fix.

@jaubourg Collaborator

The unit test makes it clear which is what matters here: we will catch the regression now. That's why I always write the unit test first then try and fix the issue, btw.

@rwaldron Collaborator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
},
deferred = {};
View
19 test/unit/deferred.js
@@ -8,7 +8,7 @@ jQuery.each( [ "", " - new operator" ], function( _, withNew ) {
test("jQuery.Deferred" + withNew, function() {
- expect( 21 );
+ expect( 23 );
var defer = createDeferred();
@@ -39,6 +39,22 @@ jQuery.each( [ "", " - new operator" ], function( _, withNew ) {
strictEqual( value , "done" , "Passed function executed" );
});
+ createDeferred(function( defer ) {
+ var promise = defer.promise(),
+ func = function() {},
+ funcPromise = defer.promise( func );
+ strictEqual( defer.promise(), promise, "promise is always the same" );
+ strictEqual( funcPromise, func, "non objects get extended" );
+ jQuery.each( promise, function( key, value ) {
+ if ( !jQuery.isFunction( promise[ key ] ) ) {
+ ok( false, key + " is a function (" + jQuery.type( promise[ key ] ) + ")" );
+ }
+ if ( promise[ key ] !== func[ key ] ) {
+ strictEqual( func[ key ], promise[ key ], key + " is the same" );
+ }
+ });
+ });
+
jQuery.expandedEach = jQuery.each;
jQuery.expandedEach( "resolve reject".split( " " ), function( _, change ) {
createDeferred( function( defer ) {
@@ -59,6 +75,7 @@ jQuery.each( [ "", " - new operator" ], function( _, withNew ) {
});
} );
+
test( "jQuery.Deferred - chainability", function() {
var defer = jQuery.Deferred();

3 comments on commit 74cdd78

@jaubourg
Collaborator

... and this was done, tested and all on the iPad \o/

/me is mobile now :)

@Krinkle

Could do with a comment making it explicit that it is intentional that all objects are supported (even objects that aren't typeof object), e.g. including function objects. Since it was a bugfix and is documented behavior.

Another common test is to verify that something is an object (including function objects) is Object( obj ) !== obj.

@rwaldron
Collaborator

@Krinkle The funny thing about this: Object( obj ) !== obj is when you use jshint it will yell about not using new (which is optional), but when you add new it yells about making objects with new Object() vs {} (not optional) :dizzy_face:

@rwaldron
Collaborator

@jaubourg ooooo cloud9?

@Krinkle

Not anymore it isn't :bowtie:

That's fixed now (jshint/jshint#392) as of JSHint r11 (2012-09-03). The native constructors are allowed to be invoked as functions to trigger the internal methods toNumber, toString, toObject etc. This was already the case for most of them, Object has joined that fix.

@jaubourg
Collaborator

@rwldrn nope, I rooted the thing. Gotta try and install flash on it to be able to use browserstack btw.

@jaubourg
Collaborator

The unit test makes it clear which is what matters here: we will catch the regression now. That's why I always write the unit test first then try and fix the issue, btw.

@rwaldron
Collaborator
Please sign in to comment.
Something went wrong with that request. Please try again.