filterFilter compare custom objects in 1.3.6

[m7r]

First off all I am a big fan of the changed behavior as I can do finally filtering complex data as I could in angular 1.0.x

Only the default comparator should not return false if actual or expected is an object.

My use case are mongodb ObjectIds with a custom toString function.
From angular 1.0 to 1.3.5 the filterFilter could compare this custom object with a string or another ObjectId.

I know I can use my one comparator but i think the new comparator is a bit to picky.
A string like '[object' is in my opinion a user error.

WDYT?

[gkalpak]

@m7r: There have been a couple of regressions reported and fixes are submitted for those, but I can't really tell if your usecase falls under one of those categories (as I can't quite understand what is the issue here).

Could you give an example (i.e. input, expected output, actual output) illustrating the problem/regression or better yet a live reproduction using plunkr / jsfiddle / ... ?

[m7r]

@gkalpak http://plnkr.co/edit/fTbKNaot13K6v81xDwRL

Switch the angular version to see better what I mean.

[gkalpak]

@m7r: I see what you mean. I personally don't think that the default comparator (which is supposed to match substrings case-insensitively) should match a string with an Object (and yes, that sounds like a good use-case for a custom comparator imo 😄), but let's see what others think.

/ping @caitp

[caitp]

We have to support objects in the default comparator, for the case when no predicate is supplied.

However, this only applies when the two items being compared are both objects, it does not compare a string with an object.

[gkalpak]

_{(Not sure what you mean by "predicate", so I assume "expression"; correct me if I'm wrong.)}

If no predicate is supplied, then the original array is returned (as was the case in pre-1.3.6). We don't need the comparator for that (since there's nothing to compare with) ?

When two we have to compare two objects they are compared by deepCompare and not the comparator.

So, if I understand correctly, you agree that we shouldn't convert objects to strings for comparing against a string ?

Note, that an object might have a custom toString method, as in @m7r's use-case.

_{Another option might be to check if the object has a `toString()` method that is not Object's `toString()` method and use it if one is found.}

[caitp]

If no predicate is supplied, then the original array is returned (as was the case in pre-1.3.6). We don't need the comparator for that (since there's nothing to compare with) ?

No, that only applies if the predicate is undefined --- and that is actually the incorrect behaviour, we need to fix that.

(Predicate, historically, was one or more comparators --- reflecting properties of the object, or custom functions --- where the result of the filter would require each predicate comparator to return true --- you've sort of mangled this a bit with your refactoring)

[gkalpak]

AFAIKT, predicate was one or more filtering functions (and there was also a comparator).
No there is the predicateFn :)

No, that only applies if the predicate is undefined

Hm...so, what do you mean by "no predicate is supplied" then ?

and that is actually the incorrect behaviour, we need to fix that.

Why is it incorrect ? What should happen instead.
(Note: We had a PR recently to make sure undefined is ignored, both as an expression and as a rpoperty value.)

[caitp]

AFAIKT, predicate was one or more filtering functions (and there was also a comparator).
No there is the predicateFn :)

You're mistaken --- the predicate functions were either constructed based on input, or were already functions. There is predicateFn, but there used to be multiple predicateFns, not just one. So the idea is, for a generated predicate, if the properties it is comparing are objects, it needs to handle them specially.

Why is it incorrect ? What should happen instead.
(Note: We had a PR recently to make sure undefined is ignored, both as an expression and as a rpoperty value.)

Because we need to reverse the array if it's asked for

Hm...so, what do you mean by "no predicate is supplied" then ?

If the value is null, you'll get a very different behaviour

[gkalpak]

Why is it incorrect ? What should happen instead.
(Note: We had a PR recently to make sure undefined is ignored, both as an expression and as a property value.)

Because we need to reverse the array if it's asked for

Hm...not sure why/how it can be asked to reverse the array. filterFilter should not change the order.

Hm...so, what do you mean by "no predicate is supplied" then ?

If the value is null, you'll get a very different behaviour

Could you give an example ? We are probably talking about different things, because I see the same behaviour whether the expression is null or undefined.

[caitp]

Hm...not sure why/how it can be asked to reverse the array. filterFilter should not change the order.

You're right, thinking of orderBy

[gkalpak]

So, @caitp, regarding the original question, I see 3 options:

1. comparator(<Object>, <Primitive>) ---> always false
   This is the current behaviour.
2. comparator(<Object>, <Primitive>) ---> comparator(<Object>.toString(), <Primitive>)
   Will incorrectly allow unexpected matches (e.g. the string object to match an Object).
3. comparator(<Object>, <Primitive>) ---> (<Object>.toString === Object.prototype.toString) ? false : /* Same as option (2) */
   Possibly combining the best of two worlds (but at a tiny extra cost).

Which on should we go by ?

[petebacondarwin]

@gkalpak - so what you are saying is that in option 3, it will return false unless the object has overridden the meaning of toString? This is kind of nice because it will also work well with Date objects.

Let's go with option 3

[petebacondarwin]

I have added the PRs plz! label to this issue as this is a fairly straightforward fix. Please make sure you do the following:

• comment on the issue to say that you are working on this issue
• sign the CLA
• add unit tests that demonstrate this problem
• implement option 3 to fix the unit tests - I would break up the current if statement that checks the type of the obj param
• update the docs to make it clear what happens if you are trying to match a string to an object
• submit a pull request with a well written commit message

[gkalpak]

@petebacondarwin: Do you mean we should leave this for a community member's first contribution (as it is a "good first PR") or do you want me to submit a PR ?

[petebacondarwin]

@gkalpak - if you have time go for it. I just wanted to put it out there as an option.

[m7r]

@gkalpak I can look into it later today too.

[petebacondarwin]

@gkalpak - let's give this one to @m7r - then perhaps you could review it?
Thanks.

[m7r]

@gkalpak could you have a look at the changes please?
I am not a native speaker are the comments and docs ok?
FYI the !isFunction is necessary for objects without prototype.

[gkalpak]

@m7r: Sure, I'll take a look tomorrow.

[gkalpak]

@m7r: I left some comments. They're mostly test-related; implementation-wise it looks good.

[m7r]

@gkalpak Thanks for the comments I have updated my commit accordingly.