Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/ng/filter/filter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@
*/
function filterFilter() {
return function(array, expression, comparator) {
if (!isArray(array)) {
if (!isArrayLike(array)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (array == null) {
return array;
} else {
Expand Down Expand Up @@ -157,7 +157,7 @@ function filterFilter() {
return array;
}

return array.filter(predicateFn);
return Array.prototype.filter.call(array, predicateFn);
};
}

Expand Down
17 changes: 17 additions & 0 deletions test/ng/filter/filterSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,23 @@ describe('Filter: filter', function() {
toThrowMinErr('filter', 'notarray', 'Expected array but received: {"toString":null,"valueOf":null}');
});

it('should not throw an error if used with an array like object', function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is obviously not sufficient. We need to test with "less array-like" objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll change the test to use arguments

function getArguments() {
return arguments;
}
var argsObj = getArguments({name: 'Misko'}, {name: 'Igor'}, {name: 'Brad'});

var nodeList = jqLite("<p><span>Misko</span><span>Igor</span><span>Brad</span></p>")[0].childNodes;
function nodeFilterPredicate(node) {
return node.innerHTML.indexOf("I") !== -1;
}

expect(filter(argsObj, 'i').length).toBe(2);
expect(filter('abc','b').length).toBe(1);
expect(filter(nodeList, nodeFilterPredicate).length).toBe(1);

});


it('should return undefined when the array is undefined', function() {
expect(filter(undefined, {})).toBeUndefined();
Expand Down