fix(filterFilter): let expression object {$: '...'} also match primitive items
#10437
Conversation
|
CLAs look good, thanks! |
| @@ -65,6 +65,26 @@ describe('Filter: filter', function() { | |||
| }); | |||
|
|
|||
|
|
|||
| it('should treat expression `{$: \'xyz\'}` as `\'xyz\'`', function() { | |||
There was a problem hiding this comment.
Suggestions on better description most welcome :)
There was a problem hiding this comment.
- Should use top-level
$property in expression object to match against primitive array values - should match primitive array values against top-level
$property in object expression
There was a problem hiding this comment.
These aren't accurate, because it will do that iff $ is the only (enumerable) property of the expression object (but nobody really cares I guess 😄).
UPDATED
There was a problem hiding this comment.
These aren't accurate, because it will do that iff $ is the only (enumerable) property of the expression object
Um, why would we care? It doesn't matter if it's the only enumerable property or not, all that matters is that it's a property at all
gkalpak
force-pushed
the
filterFilter-fix-10428
branch
from
6705b0e to
95f3869
Compare
| @@ -133,6 +133,15 @@ function filterFilter() { | |||
| //jshint -W086 | |||
| case 'object': | |||
| //jshint +W086 | |||
| if (!matchAgainstAnyProp && (expression !== null) && ('$' in expression)) { | |||
| var keysCount = 0; | |||
| for (var key in expression) { keysCount++; } | |||
|
@joshkurz: The "thing" is that we need to also account for inherited (enumerable) properties (while I.e. the following 2 cases should behave identically: // 1.
var expr = {prop1: 'val1', prop2: 'val2'};
// 2.
Object.prototype.prop1 = 'val1';
var expr = {prop2: 'val2'}; |
gkalpak
force-pushed
the
filterFilter-fix-10428
branch
from
95f3869 to
34b3dfa
Compare
| @@ -313,8 +335,6 @@ describe('Filter: filter', function() { | |||
| expect(filter(items, {}).length).toBe(1); | |||
| expect(filter(items, {})[0]).toBe(items[1]); | |||
|
|
|||
| expect(filter(items, {$: 'll'}).length).toBe(0); | |||
There was a problem hiding this comment.
Because it was failing (it expected expression to be affected by inherited properties, but in the special case of {$: ...} it isn't).
There was a problem hiding this comment.
This has nothing to do with Object.prototype (or any other prototype), $ is an own property
There was a problem hiding this comment.
If this test is failing, it means the implementation is broken
There was a problem hiding this comment.
This test is supposed to fail (in a sane world), because there is an inherited property (someProp: 'oo'). So, {$: 'll'} in this case, should be equivalent with {$: 'll', someProp: 'oo'}, which in turn (in a sane world again) means: Match any object that has a property named someProp and whose value is a superstring of oo and has any property whose value is a superstring of ll.
And because there is no such property, 0 items should be returned.
But...
There was a problem hiding this comment.
But nothing --- this change, which is not supposed to impact objects at all, is breaking this test. Thus proving that it is not the right thing to do.
gkalpak
force-pushed
the
filterFilter-fix-10428
branch
from
34b3dfa to
c7100b6
Compare
| @@ -163,6 +164,9 @@ function createPredicateFn(expression, comparator, matchAgainstAnyProp) { | |||
| } | |||
|
|
|||
| predicateFn = function(item) { | |||
| if (shouldMatchPrimitives && !isObject(item)) { | |||
There was a problem hiding this comment.
Just avoiding having to execute isObject(expression) && ('$' in expression) for every item in the array.
gkalpak
force-pushed
the
filterFilter-fix-10428
branch
from
c7100b6 to
724f335
Compare
…itive items Closes angular#10428
gkalpak
force-pushed
the
filterFilter-fix-10428
branch
from
724f335 to
d6445b5
Compare
|
lgtm I think --- @lgalfaso do you have an opinion? |
|
I think the patch is fine, let's get this into the next 1.3.x to kill the regression |
|
landed as fb2c585 |
Closes #10428