Skip to content

/ angular.js

Permalink
Browse files
Loading status checks…

fix(filterFilter): let expression object `{$: '...'}` also match prim… 

…itive items

Closes #10428
  • Loading branch information
@gkalpak @lgalfaso
gkalpak authored and lgalfaso committed Dec 12, 2014
1 parent bdbe4fd commit fb2c58589758744c0eef8c2ead3dbcf27a5cf200
Unified Split
Showing with 31 additions and 0 deletions.
  1. +4 −0 src/ng/filter/filter.js
  2. +27 −0 test/ng/filter/filterSpec.js
4 src/ng/filter/filter.js
View file
@@ -145,6 +145,7 @@ function filterFilter() {

// Helper functions for `filterFilter`
function createPredicateFn(expression, comparator, matchAgainstAnyProp) {
var shouldMatchPrimitives = isObject(expression) && ('$' in expression);
var predicateFn;

if (comparator === true) {
@@ -163,6 +164,9 @@ function createPredicateFn(expression, comparator, matchAgainstAnyProp) {
}

predicateFn = function(item) {
if (shouldMatchPrimitives && !isObject(item)) {
return deepCompare(item, expression.$, comparator, false);
}

This comment has been minimized.

Sign in to view
Copy link
@LiZhangmei

LiZhangmei Sep 10, 2015

Can we use a string '...' instead of an object {$: '...'} to implement this function, I think it is a puzzling code from line 167~169.

This comment has been minimized.

Sign in to view
Copy link
@gkalpak

gkalpak Sep 10, 2015

Author Member

In your code, you probably should indeed use a string if you want to match against primitive items (not objects).
This change (which is indeed awkward, but unavoidable) was added to restore the earlier behaviour of filterFilter, namely making an expression like {$: '...'} behave the same as '...'.

I am not sure why it was implemented like this in the first place, but changing this behaviour (even if unintuitive) would break people's apps, so we re-instated it for backwards compatibility. See the discussion on #10428 for more context.

(FWIW, I wouldn't use it in my app.)

This comment has been minimized.

Sign in to view
Copy link
@LiZhangmei

LiZhangmei Sep 10, 2015

thanks very much, I get it by reading the discussion on #10428
return deepCompare(item, expression, comparator, matchAgainstAnyProp);
};

27 test/ng/filter/filterSpec.js
View file
@@ -65,6 +65,33 @@ describe('Filter: filter', function() {
});


it('should match primitive array values against top-level `$` property in object expression',
function() {
var items, expr;

items = ['something', 'something else', 'another thing'];
expr = {$: 'some'};

This comment has been minimized.

Sign in to view
Copy link
@LiZhangmei

LiZhangmei Sep 10, 2015

not sure why not just use a simple string: expr = 'some'?

This comment has been minimized.

Sign in to view
Copy link
@gkalpak

gkalpak Sep 10, 2015

Author Member

I am not sure what you are asking.

If the question is "Why not use expr = 'some' in the test", the answer would be:
Because this test is testing the behaviour of the $ property in an object expression.

If the question is "Why support this behaviour, since on could just use 'some' instead of {$: 'some'} in their code", the answer would be:
Because the $ property can be used together with other properties.
E.g. get all toys that have some in any field: expr: {category: 'toy', $: 'some'} (Demo)

If the question is something else, then I need more clarification :)

This comment has been minimized.

Sign in to view
Copy link
@LiZhangmei

LiZhangmei Sep 10, 2015

my question is "Why support this behaviour, since on could just use 'some' instead of {$: 'some'} in their code".
What I mean is: if we need to filter some objects such as

users = [
  {id: 1, role: 'admin', firstname: 'Ann',     lastname: 'Perdan'},
  {id: 2, role: 'user',  firstname: 'Mathew',  lastname: 'Zen'},
  {id: 3, role: 'user',  firstname: 'Lynn',    lastname: 'Pociu'},
  {id: 4, role: 'user',  firstname: 'Valerie', lastname: 'Cann'}
];

we could use filter:{role: 'user', $: 'nn'}, but if we just filter a set of simple type data
such as a string array or a number array, we could just use a string like filter:'...'.
if so, we don't need to add code in src/ng/filter/filter.js#167169.
thank you for your reply

This comment has been minimized.

Sign in to view
Copy link
@gkalpak

gkalpak Sep 10, 2015

Author Member

I assume you wrote this before reading fb2c585#commitcomment-13152973 :)

Hopefully things are clear now (let me know if they aren't).

This comment has been minimized.

Sign in to view
Copy link
@LiZhangmei

LiZhangmei Sep 10, 2015

yes, thanks
expect(filter(items, expr).length).toBe(2);
expect(filter(items, expr)).toEqual([items[0], items[1]]);

items = [{val: 'something'}, {val: 'something else'}, {val: 'another thing'}];
expr = {$: 'some'};
expect(filter(items, expr).length).toBe(2);
expect(filter(items, expr)).toEqual([items[0], items[1]]);

items = [123, 456, 789];
expr = {$: 1};
expect(filter(items, expr).length).toBe(1);
expect(filter(items, expr)).toEqual([items[0]]);

items = [true, false, 'true'];
expr = {$: true, ignored: 'false'};
expect(filter(items, expr).length).toBe(2);
expect(filter(items, expr)).toEqual([items[0], items[2]]);
}
);


it('should take object as predicate', function() {
var items = [{first: 'misko', last: 'hevery'},
{first: 'adam', last: 'abrons'}];

0 comments on commit fb2c585

Please sign in to comment.
You can’t perform that action at this time.