fix(filterFilter): solve issue #10991 with null property value when using objects with filter #11116
Conversation
fix(filterFilter): solve issue angular#10991 with null property value when using objects with filter : test
… when using objects with filter fix(filterFilter): solve issue angular#10991 with null property value when using objects with filter Regarding the issue angular#10991 and the following discussion angular#10992
| expect(filter(items, f).length).toBe(3); | ||
|
|
||
| f = { people:null }; | ||
| expect(filter(items, f).length).toBe(3); |
There was a problem hiding this comment.
Hm...this test shouldn't pass (the length should be 1). But it does pass since version 1.3.6.
This made me realize that there is a similar bug with expectedType (see #11116 (comment)).
|
I left a couple of inline comments (seems that there is one more small fix to be done). @ElephantRose: I understand that your time might be limited at the moment and trying to figure out all the necessary bits for a first contribution (git stuff (forking, branching, rebasing, squashing, force-pushing), github stuff (submitting PR, updating PR), angular conventions (code style, file/folder layout, commit message formats, build/testing process), tooling (npm, grunt, gulp) etc) can be time-consuming (and frustrating at times) - although I promise it is going to be a no-brainer after a few contributions. So, there are a few things that need to be done in order for this PR to be 100% complete, but if you don't want to spend any more time on it right now, it is good enough for merging (it does solve part of the problem and squashing can be done while merging). The rest can be taken care of in subsequent PRs (I'll take care of that). In any case, thx for all the work so far 👍 |
|
@caitp or @petebacondarwin or @anyone, should this be backported to 1.3.x (since it was working correclty up until 1.3.5 and broke in 1.3.6) ? |
|
yes, backport to 1.3 |
|
@gkalpak what is the milestone in which you want to merge this? |
|
@pkozlowski-opensource, I don't really know what's the deal with milestones (nor do I know when the next release is happening (suspecting on Monday)) and how I should determine which one I want t merge this in :) If it helps answer the question, I would like to merge this as soon as possible (since it is a regression), preferrably with the additional fix mentioned above, but if not possible then as is (and submitting a follow-up PR). |
add test case for new fix
|
@gkalpak, Sorry for the late reply. I was a bit busy. |
|
@ElephantRose, thx ! Apart for the commits' needing to be squashed into one, it LGTM. |
|
@petebacondarwin, according to #11116 (comment) this should be also backported to 1.3.x |
|
@pkozlowski-opensource, @petebacondarwin, ooops ! Seems like I missed that. All good then :) |
…paring objects Closes angular#10991 Closes angular#10992 Closes angular#11116
fix(filterFilter): solve issue #10991 with null property value when using objects with filter
Regarding the issue #10991 and the following discussion #10992