each on hash object with length property fails

[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

[michaelficarra]

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.

[dvv]

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

[michaelficarra]

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

[dvv]

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

[michaelficarra]

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

[dvv]

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)

[michaelficarra]

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.

[dvv]

But that doesn't solve own .length quirk.

[michaelficarra]

@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.

[dvv]

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.

[michaelficarra]

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.

[michaelficarra]

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.

[dvv]

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.

[dvv]

Perfect

[jashkenas]

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.

[breakbild]

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

[jdalton]

I tackled 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.