fix(filter): throw error if not used with an array #10352
Conversation
|
CLAs look good, thanks! |
|
I think null/undefined should still return without error? |
|
Hi @jbedard! I'm not sure about it. Let's wait for other opinions. Cheers. |
|
Definitely not throwing on undefined/null. It is a common pattern to initialize a resource asynchronously (in which case you shouldn't have to pre-initialize it ot an empty array or object first). |
|
Yeah, actually that makes sense. |
|
since this is a breaking change (albeit a tiny one), i'm not sure we want to ship this before 1.4 --- @petebacondarwin confirm |
| '| filter:isNotFoo', | ||
| 'ng-model': 'selected' | ||
| }); | ||
|
|
There was a problem hiding this comment.
revert this and add the change that was suggested
|
So we decided we're gonna do this for 1.4 --- I think we can merge this as soon as we have moved 1.3 to its own branch, but I'm just going to take a quick look to see if there are any adjustments to be made here... |
| @@ -323,6 +323,25 @@ describe('Filter: filter', function() { | |||
| }); | |||
|
|
|||
|
|
|||
| it('should throw an error when is not used with an array', function() { | |||
| var item = {'not': 'array'}; | |||
| expect(function() { filter(item, {}); }). | |||
There was a problem hiding this comment.
I'd like some extra test cases here --- for instance, one with Object.create(null), one with a non-function toString and valueOf method --- These will work just fine currently, but if the implementation of minErr changes they might need some refactoring later on.
Otherwise it looks pretty good, I'd land this
There was a problem hiding this comment.
Hi @caitp! I've updated with three new test cases, but I'm not sure if they are what you meant. So please check it and let me know. Cheers.
There was a problem hiding this comment.
sort of --- they can be in the same it() block as the first test you wrote --- testing that it throws a specific $minErr error, rather than the TypeError that will be thrown if we change the implementation of minerr
|
|
||
|
|
||
| it('should throw an error when used with the toString() method', function() { | ||
| var item = Object.create(Object.prototype); |
There was a problem hiding this comment.
not quite :>
var item = {
toString: null,
valueOf: null,
};
expect(function() { filter(item, {}); }).
toThrowMinErr('filter', 'notarray', 'Expected array but received: {"toString": null, "valueOf": null}');
Throw error if filter is not used with an array. Closes #9992 BREAKING CHANGE: Previously, the filter was not applied if used with a non array. Now, it throws an error.
|
|
||
| This error occurs when {@link ng.filter filter} is not used with an array. | ||
| Filter must be used with an array so a subset of items can be returned. | ||
| The array can be initialized asynchronously so null or undefined won't throw this error. |
There was a problem hiding this comment.
personally I think it's good to show causes of the error in code snippets, as well as suggested fixes.
There was a problem hiding this comment.
Ok. I'll try to add it when I find time this week. Thanks!
|
lgtm, will land today |
|
a good followup bug would be to improve the error document a bit |
fixes the problem with angular 1.4 introduced by angular/angular.js#10352
Throw error if filter is not used with an array.
Closes #9992
BREAKING CHANGE: Previously, the filter was not applied if used with a non array.
Now, it throws an error.