Skip to content

/ jquery

Permalink
Browse files

Callbacks.add now accepts array-like objects (like Arguments). Now us… 

…es the slice method of the args array in fireWith rather than a quite slow jQuery.merge.
  • Loading branch information
@jaubourg
jaubourg committed Apr 25, 2012
1 parent 8cc217e commit 87c83b04589e91a96adcfce788c6207295f83a5b
Unified Split
Showing with 6 additions and 6 deletions.
  1. +6 −6 src/callbacks.js
12 src/callbacks.js
View file
@@ -94,12 +94,11 @@ jQuery.Callbacks = function( options ) {
var start = list.length;
(function add( args ) {
jQuery.each( args, function( _, arg ) {
var type;
if ( ( type = jQuery.type(arg) ) === "array" ) {
if ( jQuery.isFunction( arg ) && ( !options.unique || !self.has( arg ) ) ) {
list.push( arg );
} else if ( arg && arg.length ) {
// Inspect recursively

This comment has been minimized.

Sign in to view
Copy link
@krisselden

krisselden Oct 6, 2012

This treats every Function as an array like object (and String too but that was fixed later) and loops through prototype, so if something like say Modernizr pollyfills bind, it will add bind to the callbacks.

This comment has been minimized.

Sign in to view
Copy link
@jaubourg

jaubourg Oct 6, 2012

Author Member

Except no function is ever passed to this test. Just look at #97.

Also, jQuery doesn't support Object.prototype extensions.

This comment has been minimized.

Sign in to view
Copy link
@krisselden

krisselden Oct 6, 2012

That is simply not true ( !options.unique || !self.has( arg ) ) can fall through to else if with a function: http://jsfiddle.net/krisselden/JFSNW/

Modernizr bind is a Function.prototype extension not an Object.prototype extension and it doesn't require it being on the prototype to make it fail (see above fiddle).

The intention of the unique check I'm sure is just to prevent list.push( arg ); not to recursively add, the meaning was changed when the condition was reordered.

This comment has been minimized.

Sign in to view
Copy link
@jaubourg

jaubourg Oct 6, 2012

Author Member

Very nice catch! Could you open a ticket for that in the bug tracker so that we can tackle this properly (easy given you gave us the unit test \o/).

This comment has been minimized.

Sign in to view
Copy link
@jaubourg

jaubourg Oct 6, 2012

Author Member

Nevermind, I did it already: http://bugs.jquery.com/ticket/12665

This comment has been minimized.

Sign in to view
Copy link
@jaubourg

jaubourg Oct 6, 2012

Author Member

bb38f70
add( arg );
} else if ( type === "function" && ( !options.unique || !self.has( arg ) ) ) {
list.push( arg );
}
});
})( arguments );
@@ -121,7 +120,7 @@ jQuery.Callbacks = function( options ) {
remove: function() {
if ( list ) {
jQuery.each( arguments, function( _, arg ) {
var index = 0;
var index;
while( ( index = jQuery.inArray( arg, list, index ) ) > -1 ) {
list.splice( index, 1 );
// Handle firing indexes
@@ -170,7 +169,8 @@ jQuery.Callbacks = function( options ) {
},
// Call all callbacks with the given context and arguments
fireWith: function( context, args ) {
args = [ context, jQuery.merge( [], args || [] ) ];
args = args || [];
args = [ context, args.slice ? args.slice() : args ];

This comment has been minimized.

Sign in to view
Copy link
@gibson042

gibson042 Apr 26, 2012

Member

This seems a little fishy. Function.apply accepts undefined etc., so why manipulate args at all?

This comment has been minimized.

Sign in to view
Copy link
@jaubourg

jaubourg Apr 26, 2012

Author Member

Really?

This comment has been minimized.

Sign in to view
Copy link
@gibson042

gibson042 Apr 26, 2012

Member

Hahahaha. I just knew I was putting my foot in my mouth. But still, why the slice?

This comment has been minimized.

Sign in to view
Copy link
@jaubourg

jaubourg Apr 26, 2012

Author Member

Because without it, we have this:

var cb = $.Callbacks( "memory" ),
    myArray = [ "yes" ];

cb.fireWith( null, myArray );
myArray[ 0 ] = "err... no";
cb.add(function( value ) {
    console.log( value ); // outputs "err... no"
});

I removed don't protect for Arguments objects, because, quite franckly, the following deserves to fail:

(function() {
    var cb = $.Callbacks( "memory" ),
        myArray = arguments;

    cb.fireWith( null, myArray );
    myArray[ 0 ] = "err... no";
    cb.add(function( value ) {
        console.log( value ); // outputs "err... no"
    });
}

But maybe I should add it back once @rwaldron deals with these common decls (so that I don't have to declare yet another slice)

This comment has been minimized.

Sign in to view
Copy link
@staabm

staabm Apr 26, 2012

Contributor

@rwldrn

This comment has been minimized.

Sign in to view
Copy link
@jaubourg

jaubourg Apr 26, 2012

Author Member

Damn him and his silly username! ;)

This comment has been minimized.

Sign in to view
Copy link
@gibson042

gibson042 Apr 26, 2012

Member

You're good. That addresses both my primary question and my now unnecessary follow-up, but I'm not convinced that we should treat arrays and arguments differently. I'm with you on the common declarations; stack.push( slice.call( args || [] ) ) is definitely where we want to go in the long run (and that is acceptable because IE throws an exception on anything that isn't array/arguments... so give us a length and we'll take a list 😉).

This comment has been minimized.

Sign in to view
Copy link
@rwaldron

rwaldron Apr 26, 2012

Member

Yeah, sorry about the username non-sense. I registered as "rwldrn" a long time ago. I also have "rwaldron" but that doesn't have any of my project stuff :\

This comment has been minimized.

Sign in to view
Copy link
@jaubourg

jaubourg Apr 26, 2012

Author Member

@rwldrn yeah, I find you a little short on this one... Bwahahahahaha! ;P
if ( list && ( !fired || stack ) ) {
if ( firing ) {
stack.push( args );

0 comments on commit 87c83b0

Please sign in to comment.
You can’t perform that action at this time.