Skip to content
This repository

each on hash object with length property fails #148

Closed
breakbild opened this Issue March 17, 2011 · 18 comments

5 participants

breakbild Jeremy Ashkenas Michael Ficarra Vladimir Dronnikov John-David Dalton
breakbild

Often I use an object hash to count occurrences of tags. One of the tags could be "length".

If I later try to iterate over it with map or each it fails. The length property is interpreted as the length of the "array".

In the source I find the following:
function each, line 74
} else if (_.isNumber(obj.length)) {
for (var i = 0, l = obj.length; i < l; i++) {
if (iterator.call(context, obj[i], i, obj) === breaker) return;

by changing to: _.isArray(obj) && _.isNumber(obj.length) it works as expected

Michael Ficarra
Collaborator

The isArray(obj) test is purposely left out to allow iteration over array-like objects (arguments, for instance). It is generally a good assumption that an integer length property exists only on array-like objects.

Vladimir Dronnikov
dvv commented March 17, 2011

Having obj.length is not enough to think of obj as of iterable. Think of strings and objects like a nose ;)

Michael Ficarra
Collaborator

Ah, I was assuming this was after a check for typeof obj == "string" above (which there should be), at which point it is common to treat objects as iterable.

edit: forgot about the check that typeof obj != "function" as well

Vladimir Dronnikov
dvv commented March 17, 2011

What about var finger = { name: 'thumb', length: 2.2 }?

Michael Ficarra
Collaborator

@dvv: and how would you like to check for objects such as those?

Vladimir Dronnikov
dvv commented March 18, 2011

Namely as I wrote: having obj.length is not enough to think of obj as of iterable. Moreover, it's misleading.

L74: .isNumber(obj.length) ---> .isArray(obj)

Michael Ficarra
Collaborator

Arrays are already matched (in most situations) by nativeForEach && obj.forEach === nativeForEach. The following check is surely for array-like objects. Adding a condition that the object is not a function and either code that loops over strings with charAt or a guard against strings should be just fine.

Vladimir Dronnikov
dvv commented March 18, 2011

But that doesn't solve own .length quirk.

Michael Ficarra
Collaborator

@dvv: hint: there's no solution to that problem. Either array-like objects are supported or they aren't. You provided an array-like object.

Vladimir Dronnikov
dvv commented March 18, 2011

Correct me if I'm wrong: 0) .forEach is ES5 feature -- thus the first check is not reliable. 1) _.isArray is what we rely upon to name object an array. 2) .length is not reserved for array-like objects.

Michael Ficarra
Collaborator
  1. yep, but arrays are array-like as well
  2. not sure what you mean here
  3. also true

What kind of point were you trying to make here? I still don't see any problems with the function after applying the proposed changes.

Michael Ficarra
Collaborator
var each = _.each = _.forEach = function(obj, iterator, context) {
  if (obj == null) return;
  if (nativeForEach && obj.forEach === nativeForEach)
    return obj.forEach(iterator, context);
  if (typeof iterator != "function") return;
  if (typeof obj == "string") {
    for (var i = 0, l = obj.length; i < l; i++)
      if (iterator.call(context, obj.charAt(i), i, obj) === breaker) return;
  } else if (typeof obj != "function" && _.isNumber(obj.length)) {
    for (var i = 0, l = obj.length; i < l; i++)
      if (iterator.call(context, obj[i], i, obj) === breaker) return;
  } else {
    for (var key in obj) {
      if (!hasOwnProperty.call(obj, key)) continue;
      if (iterator.call(context, obj[key], key, obj) === breaker) return;
    }
  }
};

If this string looping is added here, it should also probably happen on _.map and anything else that iterates over objects in some way.

Vladimir Dronnikov
dvv commented March 18, 2011

A separate .safeeach() which doesn't honor .length is missing. And a BIGFATWARNING on false positives of _.each acting on object with .length in documentation I suppose.

breakbild

Maybe a separate .eachKey() to iterate over properties of an object that possibly has a .length property? I use .each only to iterate over the keys (because it's shorter). Quite useful for reflection purposes.

Vladimir Dronnikov
dvv commented March 18, 2011

Perfect

Jeremy Ashkenas
Owner

I'm afraid that the simple fact is that JavaScript objects are not suitable for use as arbitrary hashes, given the dontEnum IE bugs. You have to use arrays instead. We would prefer to have iteration over arrays and arguments objects be fast, rather than to try and make iteration safe for a few more keys in hash-like JS objects. Hence the current implementation.

This has been discussed before, so closing the ticket for the time being. We can reopen it if someone has a proposal for a performant version of each that handles arguments objects and NodeLists as well as arrays, and stops duck-typing with .length.

Jeremy Ashkenas jashkenas closed this March 20, 2011
breakbild

Sounds very reasonable. Maybe add a note about the object.length quirk in the docs, it will prevent a lot of debugging.

John-David Dalton

I solved this problem in my projects by having a generic each method that takes objects/arrays and a more specific forOwn method for those cases were my objects have length and aren't array-like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.