You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
This repository was archived by the owner on Apr 12, 2024. It is now read-only.
Repository navigation
This repository was archived by the owner on Apr 12, 2024. It is now read-only.
Updated from 1.3.2 to 1.3.6 and one of my ng-repeat filters broke.
The solution was to change item in items | filter:search to item in items | filter:search.$.
Couldn't find it in the changelog. Am I missing something?
So yeah, I think this is a mix of two problems, tbh. @gkalpak, using a build of angular with a mix of your fix and @caseyhoward's fix, the second case is still broken. I think we should use this as a test case and fix it up in one of your two PRs
@caitp: Making this works mean matching item 'some string' against expression {$: 'some string'}.
{$: 'some string'} is documented to mean "accept a match against any property of the object". The {...} expression format is intended for matching against object items (which have properties), not against strings.
IMO, opinion, it makes absolutely no sense to have an object expression match against primitive types.
@caitp: Been wondering: filterFilter used to work incorrectly and in many cases differently than documented (e.g. this issue). #9757 was about making it right (i.e. make it work as documented for cases that were documented and make it work reasonably and consistently across nesting levels for undocumented cases).
Unavoidably, people have been depending on the broken behaviour of filterFilter. We can't fix it and retain full backward compatibility at the same time. So, if the plan is to revert to the broken behaviour piece by piece, it might make more sense to revert #9757 altogether (and maybe land it on 1.4 or something).
Regarding this particular case:
If {$: 'xyz'} should match 'xyz', doesn't it mean that {level1: {$: 'xyz'}} should match {level1: 'xyz'} and so on ? This seems really weird (even if people depend on it 😄).
So, you say we should have a top-level {$: 'xyx'} also match primitive values, but a non-top-level {$: 'xyz'} only match objects ?
Sure, makes sense !
Then we only need to break deep object filtering and we'll have our nice pre-1.3.6 implementation (with a little more readable code maybe) !
I don't think we need to break deep object filtering, we just need to unbreak peoples applications. please stop misreading this as an attack on your patch --- it was very hard to review that thing, and we obviously were missing test cases so it wasn't obvious that we were breaking things. But we need to get peoples code working again.
You can add comments like // TODO(gkalpak): remove this line in 1.4 so that we know to get rid of it later, when we CAN make breaking changes.
Activity
caitp commentedon Dec 12, 2014
This is a regression, Fix is already in the works. Would get the issue # but on mobile
caitp commentedon Dec 12, 2014
So yeah, I think this is a mix of two problems, tbh. @gkalpak, using a build of angular with a mix of your fix and @caseyhoward's fix, the second case is still broken. I think we should use this as a test case and fix it up in one of your two PRs
gkalpak commentedon Dec 12, 2014
@caitp: Making this works mean matching item
'some string'against expression{$: 'some string'}.{$: 'some string'}is documented to mean "accept a match against any property of the object". The{...}expression format is intended for matching against object items (which have properties), not against strings.IMO, opinion, it makes absolutely no sense to have an object expression match against primitive types.
WDYT ?
caitp commentedon Dec 12, 2014
I think if people depended on it, we should fix it
gkalpak commentedon Dec 12, 2014
@caitp: Been wondering:
filterFilterused to work incorrectly and in many cases differently than documented (e.g. this issue).#9757 was about making it right (i.e. make it work as documented for cases that were documented and make it work reasonably and consistently across nesting levels for undocumented cases).
Unavoidably, people have been depending on the broken behaviour of
filterFilter. We can't fix it and retain full backward compatibility at the same time. So, if the plan is to revert to the broken behaviour piece by piece, it might make more sense to revert #9757 altogether (and maybe land it on 1.4 or something).Regarding this particular case:
If
{$: 'xyz'}should match'xyz', doesn't it mean that{level1: {$: 'xyz'}}should match{level1: 'xyz'}and so on ? This seems really weird (even if people depend on it 😄).caitp commentedon Dec 12, 2014
I think the top-level
$could apply to primitive values, since it used togkalpak commentedon Dec 12, 2014
So, you say we should have a top-level
Sure, makes sense ! Then we only need to break deep object filtering and we'll have our nice pre-1.3.6 implementation (with a little more readable code maybe) !{$: 'xyx'}also match primitive values, but a non-top-level{$: 'xyz'}only match objects ?caitp commentedon Dec 12, 2014
I don't think we need to break deep object filtering, we just need to unbreak peoples applications. please stop misreading this as an attack on your patch --- it was very hard to review that thing, and we obviously were missing test cases so it wasn't obvious that we were breaking things. But we need to get peoples code working again.
You can add comments like
// TODO(gkalpak): remove this line in 1.4so that we know to get rid of it later, when we CAN make breaking changes.gkalpak commentedon Dec 12, 2014
Sorry, if I sound like this. I don't mean to 😇 I certainly don't read anything as an attack on my patch.
I'll submit a fix soon.
fix(filterFilter): let expression object `{$: '...'}` also match prim…
fix(filterFilter): let expression object `{$: '...'}` also match prim…
fix(filterFilter): let expression object `{$: '...'}` also match prim…
fix(filterFilter): let expression object `{$: '...'}` also match prim…
fix(filterFilter): let expression object `{$: '...'}` also match prim…
fix(filterFilter): let expression object `{$: '...'}` also match prim…
fix(filterFilter): let expression object `{$: '...'}` also match prim…