better handling of Collection types #799

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

afeld commented Sep 27, 2012

When working on Backbone-Nested I noticed that I wasn't able to iterate over an object with a length property set to an arbitrary value, which is present in the Backbone tests.

_.each({length: 20}, function(){ /* never reached */ });

_.each() and _.size() were handing this case incorrectly, and _.isEmpty() was additionally giving incorrect results for Arguments, HTMLCollections, NodeLists and jQuery Array-likes. I have added _.isArrayLike() so that these methods (which all depend on .length) can detect the types in a central place.

Tests are green in Chrome 22, FF 15 and Safari 6.0. I did some testing in various IE versions as I went but used up my Browserstack limit, so if others wouldn't mind verifying (particularly v6-8), that would be appreciated. Thanks!

better handling of Collection types
Methods that operate on Collections (_.each(), _.size(), _.isEmpty(),
etc.) are now more consistent for Arguments, HTMLCollections, NodeLists,
and jQuery Array-likes, as well as ensuring that objects with an
arbitrary "length" property are not treated as array-like.  Added an
_.isArrayLike() method for detection.

afeld commented Sep 27, 2012

This addresses #448 #252 #659 #741 #148 #708 #724 #690 #784 #496 #653 #708. Phew.

Contributor

jdalton commented Sep 27, 2012

I've handled this using _.forOwn and _.forIn and a more generic check in _.isEmpty without having to resort to extra function calls in collections methods or special casing jQuery/DOM collections.

Owner

jashkenas commented Sep 27, 2012

Thanks for the lovely patch, but, as discussed in many previous tickets (as you listed ;) ) our desired semantic check for "array-like" is "numeric length property". We don't want to put all of those special case checks into hot loops, or to miss out on other array-like objects that aren't whitelisted.

@jashkenas jashkenas closed this Sep 27, 2012

afeld commented Sep 27, 2012

If that's the case, it should be noted in the docs to be careful of adding a length property to objects. Also, if the final decision is to always use the length, _.isEmpty() isn't consistent with this.

IonDen commented Sep 27, 2012

îÕ ×ÓÅ, Ñ ÐÏÛÅÌØ) äÏ ×ÓÔÒÅÞÉ × ÐÏÅÚÄÅ!

2012/9/27 Aidan Feldman notifications@github.com

If that's the case, it should be noted in the docs to be careful of adding
a length property to objects. Also, if the final decision is to always
use the length, _.isEmpty() isn't consistent with thishttps://github.com/documentcloud/underscore/blob/1.3.3/underscore.js#L778
.

Reply to this email directly or view it on GitHubhttps://github.com/documentcloud/underscore/pull/799#issuecomment-8940695.

äÅÎÉÓ éÎÅÛÉÎ

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