fix(isArrayLike): Correct logic in isArrayLike to screen out objects

[wesleycho]

• Fix ngRepeat check via isArrayLike check to prevent objects from
  accidentally passing as array-like

This addresses #8000 & #4751.

[googlebot]

CLAs look good, thanks!

[chirayuk]

I'm not sure if this is a good check.  For instance, consider the following expression:

(function () { return isArrayLike(arguments); })()

With your fix, that evaluates to false but I think it should return true as it used to before.

Would you improve the check?

[shahata]

I propose we change this to (length === 0 && Object.keys(obj).length <= 1)

[gkalpak]

How about (length === 0 && Object.keys(obj).length <= 1) (to account for cases where length is non-enumerable) ?

[shahata]

Yes, I already updated my comment to do this. I just checked in Chrome, Firefox and IE9/10/11 - all of them print 0 for the following:

(function () { console.log(Object.keys(arguments).length); })()

[chirayuk]

Looking into it some more, I don't think we can really distinguish between an arguments object of length 0 and {length: 0}.  Getting the behavior right for the arguments object isn't that important.  However, I want to make sure that we don't break array like classes that might be defined by user code.  I need to think some more about that (e.g. Closure library's classes that might be array like.)

[wesleycho]

You are correct it looks like as far as I can tell from this article, unless we are not in strict mode.

I am not sure use of arguments would be a good object to use in ng-repeat though. It would be a vastly inefficient expression, if in the event it doesn't throw an infinite digest error due to the arguments object being new with each evaluation of the expression.

Whatever the decision made, it probably is worthwhile updating the documentation to note the chosen behavior.

[pkozlowski-opensource]

@chirayuk seems like we are hitting the same bug in $http (#10272). Did you have a chance to check things on the closure side?

[caitp]

In the ES spec, usually when we're dealing with maybe-array-like-objects, we only care about the "length" property --- and even then, we don't care that much about it (EG, we will coerce it to a non-negative integer < 2^53-1, and it will default to 0 if it wasn't there at all)

Similar logic might look like this:

if (obj !== null && typeof obj === "object") { // if Type(O) is Object, roughly
  if (Array.isArray(obj)) {
    // O is an exotic Array object
    return true;
  } else {
    // In all other cases, we're basically going to coerce properties without a `length`
    // key to an empty array --- but for our purposes, we can pretty much do this:
    return obj.length === "number";

    // If you really care that it's already an integer and is already a valid Array index, you
    // could also check that obj.length === obj.length | 0, and that obj.length >= 0, and
    // that obj.length < 2^32 (for Array indexes) or 2^53 (for the maximum length values)
  }
}

[wesleycho]

I just refactored according to the suggestion made by @shahata.

[wesleycho]

The last commit in master has a broken unit test - my second commit fixes it, in the event it is desired to cherry-pick it over in order to fix the Travis build.

I rebased this PR off of master (probably unnecessary).

[gkalpak]

isDefined(value) would be more straightforward 😄
(Or just value != null, but I prefer the explicitness of the 1st alternative.)

Although this is a valid fix, I think it's better to have it in a separate PR, since it is totally unrelated to this PR.

[caitp]

@wesleycho I don't think this is right --- anything with a length property is going to look array-like, there isn't much we can do about that. It might look like a very holey array, but that doesn't really matter. TC39 deals with this, so should we.

Since we target older browsers, we can't rely on @@iterator, which might be a better test, so I think just testing for the presence of length makes the most sense.

[wesleycho]

So then should this PR just be closed then?

[caitp]

probably --- one workaround for them would be some config option to require ngRepeat to use arrays or something, which wouldn't really solve their problem but would at least allow them to work around it easier

[shahata]

Just my 2 cents... It seems that this does not only effect ngRepeat, it also effects $http (#10272) and angular.extend() (#4751) and I'm pretty sure there are more examples we don't know about yet. I guess we can supply a workaround for all of those but the important thing is not just to have a way to make it work. The important thing is not to let people waste precious time on debugging this and be surprised by this behavior. We already have an extra check for (length - 1) in obj and we don't only rely on the presence of the length property. I think it will be a good idea to add the Object.keys(obj).length <= 1 check.