Skip to content
Permalink
Browse files
Make sure that empty nodelists continue to map properly. Fixes #8993.
  • Loading branch information
jeresig committed May 2, 2011
1 parent 86aa764 commit 6c449fd
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 2 deletions.
@@ -720,7 +720,7 @@ jQuery.extend({
i = 0,
length = elems.length,
// jquery objects are treated as arrays
isArray = elems instanceof jQuery || length !== undefined && typeof length === "number" && ( ( length > 0 && elems[ 0 ] && elems[ length -1 ] ) || jQuery.isArray( elems ) ) ;
isArray = elems instanceof jQuery || length !== undefined && typeof length === "number" && ( ( length > 0 && elems[ 0 ] && elems[ length -1 ] ) || length === 0 || jQuery.isArray( elems ) ) ;

// Go through the array, translating each of the items to their
if ( isArray ) {
@@ -653,7 +653,7 @@ test("first()/last()", function() {
});

test("map()", function() {
expect(7);
expect(8);

same(
jQuery("#ap").map(function(){
@@ -694,6 +694,12 @@ test("map()", function() {
});
equals( mapped.length, scripts.length, "Map an array(-like) to a hash" );

var nonsense = document.getElementsByTagName("asdf");
var mapped = jQuery.map( nonsense, function( v, k ){
return v;
});
equals( mapped.length, nonsense.length, "Map an empty array(-like) to a hash" );

var flat = jQuery.map( Array(4), function( v, k ){
return k % 2 ? k : [k,k,k];//try mixing array and regular returns
});

18 comments on commit 6c449fd

@rkatic
Copy link
Contributor

@rkatic rkatic commented on 6c449fd May 2, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem of simply adding length === 0 is that now even functions with no args (length == 0) will be traversed as arrays.
I know there is no such unit test (to add?), but $.each has.

@timmywil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rkatic: I'm not sure what you mean. Passing a function to map?

@kflorence
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I guess John isn't in favor of isArrayLike either?

@rkatic
Copy link
Contributor

@rkatic rkatic commented on 6c449fd May 2, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timmywil: Yes. Like in $.each.

@kflorence: Maybe because of number of lines?

@timmywil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not trying to be difficult, but you mean iterate a function? A code example would help me understand.

Also, we tabled isArrayLike for 1.7. I'm not sure John's had time to review the matter.

@rkatic
Copy link
Contributor

@rkatic rkatic commented on 6c449fd May 2, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. For example $.map( MyClass, handleStatics ).

EDIT: Probably not a big deal. Probably I am just a consistency-freak...

@timmywil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I don't think that's a valid use case either for map or for each. I think iterating an instance(as in the case of a jQuery object) would be workable, but not a raw function.

@rkatic
Copy link
Contributor

@rkatic rkatic commented on 6c449fd May 2, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$.each does supports iterating functions. Just look in $.each code and you will see isObj = length === undefined || jQuery.isFunction( object );.
Also there is a respective unit test for $.each.

EDIT by @mgol: Fixed code formatting

@timmywil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I thought you were suggesting there was a regression in map.

@rkatic
Copy link
Contributor

@rkatic rkatic commented on 6c449fd May 2, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said, probably I am just a consistency-freak...

and for that I am glad that isArrayLike will be considered. :)

@timmywil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough :)

@kflorence
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said, probably I am just a consistency-freak...
and for that I am glad that isArrayLike will be considered. :)

Me too. Glad to hear it will be considered in 1.7!

@jboesch
Copy link
Contributor

@jboesch jboesch commented on 6c449fd May 3, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kinda glad this is consistent. I know we don't say that jQuery.map supports node lists in the docs, but we had a unit test in core.js that mapped through node lists. This should have either been removed from the unit tests (if we chose not to support node lists) or added support to empty node lists.

@rkatic
Copy link
Contributor

@rkatic rkatic commented on 6c449fd May 3, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jboesch: Rarely you can say that a documentation is also a specification for something. First one is generally a more user-friendly version of the second.
In case of jQuery, documentation is certainly not the specification. Is it needed? Maybe, but it is not simple in case of jQuery... but unit tests are pretty close.

EDIT: Also, i think NodeList are also objects. Why you think it have to be explicitly mentioned to be supported?

@jboesch
Copy link
Contributor

@jboesch jboesch commented on 6c449fd May 3, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rkatic: The docs are the only specification there is at the moment (that I'm aware of). All I'm sayin' is that I think jQuery should either support node lists and go all out (which means handling the empty node lists properly and documenting it), or don't support node lists at all.

@rkatic
Copy link
Contributor

@rkatic rkatic commented on 6c449fd May 3, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I updated my previous comment: Why NodeLists have to be explicitly mentioned if those are also objects?

@jboesch
Copy link
Contributor

@jboesch jboesch commented on 6c449fd May 3, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A node list looks like an object, but technically, it's a node list (with an item method).
You're right about being able to handle them like objects though. Maybe it doesn't need to be documented. shrug

@rkatic
Copy link
Contributor

@rkatic rkatic commented on 6c449fd May 3, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For users, it probably don't have to be mentioned (it's a documentation). For developers there are unit tests (+docs) :)

Please sign in to comment.