fix(filterFilter): allow array like objects to be filtered

[gonzaloruizdevilla]

Throw error if filter is not used with an array like object.

Previous PR #10352 restricted filterFilter to use only arrays, but it can perfectly work with array like objects.

Closes #11782

[gkalpak]

This is not sufficient. We are using array.filter, which isArrayLike does not guarrantee to exist.

Maybe we could change array.filter(...) with something like isArray(array) ? array.filter(...) : Array.prototype.filter.call(array, ...).
It might even be better to always use the latter form regardless of the type of array.

This needs a little more investigation.

[gonzaloruizdevilla]

I think is better to use the latter form always.

I made this microbench: http://jsperf.com/with-or-without-isarray
Even assuming that using arrays is the most common case, I don't think that the performance gain is worth having the check.

[gkalpak]

I've updated the jsperf (http://jsperf.com/with-or-without-isarray/2) to reuse Array.prototype.filter (although that should hardly make any noticable difference).

Basically, 99% of the time, I believe we will be dealing with arrays, so that is the usecase we should be optimizing for. So, we are mainly concerned about array with check vs array without check (using Array.prototype.filter).
Indeed, on desktop Chrome and Firefox, both seem to be almost equally fast.
On mobile Chrome, the latter was about 10% slower.
More importantly, on desktop IE11, the latter was consistently >15% slower.

So, I would go with the former.

---

We should:

1.) Store Array.prototype.filter (if nothing else for readability)
2.) Allow array-like objects.
3.) Check array and decide what function to call.

var arrayFilter = Array.prototype.filter;
...
if (!isArrayLike(array)) { /* Throw */ }
...
return isArray(array) ? array.filter(predicateFn) : arrayFilter.call(array, predicateFn);

[gonzaloruizdevilla]

Because of my poor english I normally don't write down my arguments too well.
I'll try to explain my point a little more (however, and of course, I don't have any problem to change the PR to make the check).

In the jsperf, we see very little difference given between array with check and array without check that:

• we have very few elements
• most important, the filterFn is super simple and gets superoptimized by the compiler

In a more realistic scenario where we may be concerned by performance, I think we'll have this differences:

• the number of elements will be two orders of magnitude bigger or more
• the predicateFn will be one created by createPredicateFn, that will be way slower than the one optimized in the jsbin

By this reasons, IMHO, I think that the improvement of the check is so small when performance is an issue, that is not worth it.

But again, if you think it is worth it, I'll change the PR :)

[gkalpak]

Yes, I think you are right. It's fine as it is.

[gkalpak]

Previous PR #10352 restricted filterFilter to use only arrays, but it can perfectly work with array like objects.

A little correction: filterFilter has not been supporting array-like objects (in the broad sense) since forever.
It did support objects prototypally inheriting from Array up until v1.1.2.
Since then it was just returning the original, non-array object unchanged.
All PR #10352 did, was throw an error instead of silently returning the original object (no functionality was changed).

[gkalpak]

We should also have tests for as many array-like objects as possible.
(By "array-like" I mean objects for which isArrayLike() would return true.)

[gkalpak]

I would like to have tests for more types of array-like objects (e.g. strings, jQuery objects etc).
But it's not a blocker. I would be glad landing it as is too.

Thx @gonzaloruizdevilla !

[gonzaloruizdevilla]

@gkalpak I've added more checks in the test, for both string and a nodelist. As expected, the source of filter.js remained unchanged with these additions.

[gkalpak]

LGTM
(The commit message should be changed to feat instead of fix, but that can be done during merging.)

Thx @gonzaloruizdevilla

[petebacondarwin]

It was not immediately clear to me what was being tested here, i.e. the array-like arguments object. It would be better to write something like:

function getArguments() {
  return arguments;
}
var argsObj = getArguments({name: 'Misko'}, {name: 'Igor'}, {name: 'Brad'});
expect(filter(argsObj, 'i').length).toBe(2);

[petebacondarwin]

I think we can merge this with the change to the arguments test; although I wonder if an even less array-like object would fail when passed to Array.prototype.filter?

[gkalpak]

The "arraylike-ness" of the input is determined using isArrayLike(), so it's very possible to have something non array-ish pass as array-like as long as it passes the following test:

typeof length === 'number' && length > 0 && (length - 1) in obj

But it doesn't seem to break Array.prototype.filter (in Chrome at least). E.g.:

var obj = {length: 10, k1: 'v1', k2: 'v2', 9: 'test'};
Array.prototype.filter.call(obj, function () { return true; });   // results in: ['test']

Other than that, isArrayLike recognizes objects with length: 0 (which also doesn't pose any problems to Array.prototype.filter) or arrays, strings and node-lists (all of which are tested against).

Besides, even if there is a way to break it (and you try really hard), you might end up with an error.
But this is what you would end up with previously as well. (filterFilter would throw an error if passed an non-array input.)

[petebacondarwin]

Awesome let's go with it!

[gonzaloruizdevilla]

Trying really hard 😄, the only thing I've achieved is to kill the performance with an object like {0:"a",1000000000:"c",length:1000000001, someotherProperty:"x"}, which looks very far-fetched to me.

@petebacondarwin Thanks for your suggestion, it is much more readable now.

I've updated the PR and the commit message.

[petebacondarwin]

LGTM