Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

@gonzaloruizdevilla
Copy link
Contributor

@gonzaloruizdevilla gonzaloruizdevilla commented May 1, 2015

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

@@ -127,7 +127,7 @@
*/
function filterFilter() {
return function(array, expression, comparator) {
if (!isArray(array)) {
if (!isArrayLike(array)) {

This comment has been minimized.

@gkalpak

gkalpak May 1, 2015
Member

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.

This comment has been minimized.

@gonzaloruizdevilla

gonzaloruizdevilla May 1, 2015
Author Contributor

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.

This comment has been minimized.

@gkalpak

gkalpak May 2, 2015
Member

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

This comment has been minimized.

@gonzaloruizdevilla

gonzaloruizdevilla May 2, 2015
Author Contributor

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

This comment has been minimized.

@gkalpak

gkalpak May 6, 2015
Member

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

@@ -425,6 +425,20 @@ 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() {

This comment has been minimized.

@gkalpak

gkalpak May 1, 2015
Member

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

This comment has been minimized.

@gonzaloruizdevilla

gonzaloruizdevilla May 1, 2015
Author Contributor

i'll change the test to use arguments

@@ -463,6 +477,8 @@ describe('Filter: filter', function() {
});



This comment has been minimized.

@gkalpak

gkalpak May 1, 2015
Member

These 2 extra lines seem redundant.

This comment has been minimized.

@gonzaloruizdevilla

gonzaloruizdevilla May 1, 2015
Author Contributor

oops, sorry, my fault

@petebacondarwin petebacondarwin modified the milestone: 1.4.x - jaracimrman-existence May 1, 2015
@gonzaloruizdevilla gonzaloruizdevilla force-pushed the gonzaloruizdevilla:issue-11782 branch from e09ed5d to 419f560 May 1, 2015
@gkalpak
Copy link
Member

@gkalpak gkalpak commented May 2, 2015

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
Copy link
Member

@gkalpak gkalpak commented May 2, 2015

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 gkalpak self-assigned this May 2, 2015
@gkalpak
Copy link
Member

@gkalpak gkalpak commented May 6, 2015

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 gonzaloruizdevilla force-pushed the gonzaloruizdevilla:issue-11782 branch from 419f560 to 7115bd1 May 16, 2015
@gonzaloruizdevilla
Copy link
Contributor Author

@gonzaloruizdevilla gonzaloruizdevilla commented May 16, 2015

@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
Copy link
Member

@gkalpak gkalpak commented May 18, 2015

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

Thx @gonzaloruizdevilla

}


aux({name: 'Misko'}, {name: 'Igor'}, {name: 'Brad'});

This comment has been minimized.

@petebacondarwin

petebacondarwin May 18, 2015
Member

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
Copy link
Member

@petebacondarwin petebacondarwin commented May 18, 2015

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
Copy link
Member

@gkalpak gkalpak commented May 18, 2015

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
Copy link
Member

@petebacondarwin petebacondarwin commented May 18, 2015

Awesome let's go with it!

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

Closes #11782
@gonzaloruizdevilla gonzaloruizdevilla force-pushed the gonzaloruizdevilla:issue-11782 branch from f4cdcf0 to 42b6453 May 19, 2015
@gonzaloruizdevilla
Copy link
Contributor Author

@gonzaloruizdevilla gonzaloruizdevilla commented May 19, 2015

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
Copy link
Member

@petebacondarwin petebacondarwin commented May 19, 2015

LGTM

@gkalpak gkalpak closed this in 1b0d0fd May 19, 2015
@gonzaloruizdevilla gonzaloruizdevilla deleted the gonzaloruizdevilla:issue-11782 branch May 22, 2015
netman92 added a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
Throw error if filter is not used with an array like object.

Closes angular#11782
Closes angular#11787
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants