Adding object support to $.map #238

Closed
wants to merge 6 commits into from

2 participants

@jboesch

Opened a new ticket: http://bugs.jquery.com/ticket/8328

Old tickets that mention this: http://bugs.jquery.com/ticket/2616 and http://bugs.jquery.com/ticket/4459

Since it was 3 years ago, it didn't seem like it's a huge priority. I wanted to make the addition to handle objects without sacrificing the raw speed of $.map for arrays. Underscore.js already implements this: http://documentcloud.github.com/underscore/#map

Here are some benchmarks: http://jsperf.com/old-map-vs-new-map-with-obj-support/2

The pull request contains test: $.new_map_forin_2.

Some more tests here: http://jsfiddle.net/mmhwZ/16/

@rwaldron
jQuery Foundation member

Instead of the length === undefined stored as isObj, why not use jQuery.type( elems).. Because I might have an object with a length property. Also, the indentation of the if block is to far

@jboesch

Cool. Yeah I was thinking of using isPlainObject, but that's a little slower. I checked jQuery.each and saw it was using
isObj = length === undefined; so I figured I would just use the same. Should it not be changed in jQuery.each as well? I figured it hadn't been changed because it's less performant to do an actual object check using isPlainObject

I tried changing it to jQuery.type( elems ) == 'object' but it seemed to crash the unit tests :p

I will patch it using isPlainObject

@jboesch

Hmm.. isPlainObject is less performant... upon more thinkerizing...
I wonder if having the "length === undefined" is sufficient for speed purposes... seems like an edge case where someone would pass an object with a key of "length". Having a "length" key currently does not work with the current $.each implementation: http://jsfiddle.net/jboesch26/HRapv/1/

Looking for feedback on this :)

@rwaldron
jQuery Foundation member

Code solution aside, the if block is still indented too far.

@jboesch

re-opening pull request under a proper branch

@jboesch

This pull request can be found here: #252

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment