Update filter.js

[ElephantRose]

Solve case : #10991
with using filter and deep object

[googlebot]

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

[ElephantRose]

Just signed a Contributor License Agreement (CLA)

[googlebot]

CLAs look good, thanks!

[gkalpak]

Nice catch, but I don't think the fix is right. The problem is that typeof null returns object which results in following the object path which is not the right thing to do. Just comparing against actualVal if actulaVal is not semantically the same as following the "non-object-nor-function" path, namely delegating comparison to the comparator.

So, the right fix would involve differentiating "real" objects from null.

I would probably change:

var actualType = typeof actual;

with:

var actualType = (actual !== null) ? typeof actual : 'null';   // anything other than "object" or "function"

[gkalpak]

@ElephantRose: BTW, it would also be great to have a couple of tests should in order to ensure that this is not accidentally broken in the future.

[ElephantRose]

@gkalpak

Hi,

Thanks for your reply.
I just tried your fix and i found a small issue
When I clear the filter's input field, the object with null property value are still filtered (and so not displayed)
It's because the filter value for the user name is not reset to null, but is put to an empty string

Here is a jsfiddle demo : http://jsfiddle.net/ykm0d6f0/13/

The 1rst case, is without fix just for reference
The 2nd one use my first solution
And the 3rd one use yours, as you can see the last row (the one with null value) will not be displayed anymore if you enter a user name value.

Sadly I do not have other tests, so i can't guarantee what i proposed work for every cases.

[gkalpak]

@ElephantRose: Unfortunately, this is a different issue (not related to this one).
Here is what is going on:

• Before the input is touched, there is no prop1 defined on f, so the filtering expression is {}.
• After, interracting with the input (and emptying it again) the filtering expression (f) already has this structure: {prop1: {prop2: xyz}}. No matter what you put into xyz ('', null, undefined) it won't match against {prop1: null}.

This is why I said that your original fix changes the semantics of filterFilter.
Whether we want to support a way to make some attributes "optional" (as in "ignored if not present") is another story.

So, to recap, there are 2 different issues discussed here:

1. Treat null as object (and trying to access properties on it).
   This is clearly a bug and should be corrected. See Update filter.js #10992 (comment).
2. Match item {prop1: null} against filtering expression {prop1: {...}}.
   This is not a bug, but rather a "feature" request and would need a breaking change if we decided to support it. And even if we did, a different approach would be needed (than the one proposed in this PR).

_{Regarding (2) (off the top of my head), it might be possible to prefix/suffix a property name with ? to make it optional (not sure if ? in a property name would pose any problems in any of the supported browser).}

[gkalpak]

So, here is what I propose:

1. Update this PR with the fix mentioned above.
2. Use a custom comparator to solve your problem Filter : issue when some properties are null (deep object filter) #10991 (i.e. a comparator that matches null against anything).
3. If you still think that having "optional" properties is a feature that should be natively supported by filterFilter, open a separate (feature request) issue.

[ElephantRose]

@gkalpak

I understand the 2nd point is not really an issue... more a side effect.
Frankly i can't even tell myself in that particulary case if a property with an empty value should be treated (or not) the same way than a null value.

I'll definitely look after custom comparator before opening a feature request.

Thanks for your help

[gkalpak]

@ElephantRose, so since this PR is not the right fix, would you be interested in submitting a new PR to fix the null issue (based on #10992 (comment)) ?
_{(If not I will gladly do it myself :))}

[ElephantRose]

@gkalpak, I'll be glad to do a new one

And I didn't reply about my 2nd point, but i managed to do it with custom comparator as you suggested.