fix(orderBy): Throw exception when orderBy is given a non array-like object. #11719

nhodges · 2015-04-24T21:42:42Z

Throws exception when orderBy is given a non array-like object to sort.

petebacondarwin · 2015-06-30T01:29:11Z

src/ng/filter/orderBy.js

@@ -155,7 +155,7 @@
 orderByFilter.$inject = ['$parse'];
 function orderByFilter($parse) {
  return function(array, sortPredicate, reverseOrder) {
-    if (!(isArrayLike(array))) return array;
+    if (!(isArrayLike(array))) throw minErr('filter')('notarray', 'Expected array but received: {0}', array);


Sadly this error code is specific to the filterFilter so we have to provide a new one and associated documentation in docs/content/error or alternatively update the documentation to make this error code more generic.

fhernandezn · 2015-10-08T03:53:00Z

@nhodges @petebacondarwin Hi guys, I've been working in the documentation needed for this pull request. Please look at this

master...frankweb:orderby-notarray-documentation

I'd like to contribute to this pull request by providing documentation.

nhodges · 2015-10-12T22:06:01Z

👍 @Frankweb for providing documentation, I updated my PR to be fast forwardable.

fhernandezn · 2015-10-13T03:55:04Z

@nhodges 👍 do you want to include my documentation in your PR?

googlebot · 2015-10-13T17:25:20Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

nhodges · 2015-10-13T17:25:54Z

@Frankweb You got it :)

nhodges · 2015-10-13T17:34:57Z

@Frankweb looks like the task to build the docs is failing after merging, can you take a look?

fhernandezn · 2015-10-13T17:35:48Z

@nhodges Of course, I'll take a look in a while :)

fhernandezn · 2015-10-13T18:26:02Z

src/ng/filter/orderBy.js

@@ -177,7 +177,7 @@ orderByFilter.$inject = ['$parse'];
 function orderByFilter($parse) {
  return function(array, sortPredicate, reverseOrder) {

-    if (!(isArrayLike(array))) return array;
+    if (!(isArrayLike(array))) throw minErr('filter')('notarray', 'Expected array but received: {0}', array);


grunt test runs without errors if I change minErr('filter') to minErr('orderBy')

nhodges · 2015-10-14T19:49:55Z

👍 @Frankweb thanks for the heads up. Fixed and we have a green build again.

gkalpak · 2015-10-15T14:17:24Z

@nhodges, could you take care of the unresolved comments ?

gkalpak · 2015-10-15T14:24:14Z

docs/content/error/orderBy/notarray.ngdoc

+             },
+      three: {
+             name: 'something 3'
+             },


Also, the indentation is funny for this object. TBH, I would change the example to something like:

 <label> Order by: <select ng-model="orderProp" ng-options="prop for prop in ['id', 'name']"></select> </label> <div ng-repeat="(key, value) in myObj | orderBy:orderProp"> {{ key }} : {{ value }} </div>

/* JS */ angular.module('aModule', []) .controller('aController', function($scope) { var myObj = { one: {id: 1, name: 'Some thing'}, two: {id: 2, name: 'Another thing'}, three: {id: 3, name: 'A third thing'} }; $scope.arrFromMyObj = Object.keys(myObj).map(function(key) { return myObj[key]; }); });

nhodges · 2015-10-15T17:22:28Z

@gkalpak You got it. Pushed.

nhodges · 2015-10-19T23:42:33Z

@gkalpak @petebacondarwin @Frankweb

Anything else I can take care of?

fhernandezn · 2015-10-20T00:32:53Z

@nhodges Sorry for the last comment, I already deleted it, my comment was about other PR.

fhernandezn · 2015-10-20T00:36:43Z

There is only one problem with the PR, I think all the commits should be made from one account, since you merged my branch this PR has commits with two accounts. Because of this, cla label says no @Narretz @petebacondarwin should @nhodges do another PR or what should he do?

nhodges · 2015-10-20T00:52:51Z

Hey Francisco,

I think if you just sign the CLA should be good?

Made with thumbs.

On Oct 19, 2015, at 5:38 PM, Francisco Hernández notifications@github.com wrote:

There is only one problem with the PR, I think all the commits should be made from one account, since you merged my branch this PR has commits with two accounts. Because of this, cla label says no @Narretz @petebacondarwin should @nhodges do another PR or what should he do?

—
Reply to this email directly or view it on GitHub.

fhernandezn · 2015-10-20T01:10:07Z

@nhodges I guess we should comment something to the googlebot, but I don't know exactly what.

fhernandezn · 2015-10-20T01:11:41Z

@googlebot I'm okay with these commits.

fhernandezn · 2015-10-20T17:38:03Z

@googlebot I authored these commits

nhodges · 2015-10-20T19:13:10Z

@Frankweb

Hey Francisco, can you try clicking the link below and filling out the form?

http://code.google.com/legal/individual-cla-v1.0.html

Thanks!

fhernandezn · 2015-10-20T19:18:12Z

@nhodges Of course, but I'm already signed in. I'll try again.

petebacondarwin · 2015-10-22T20:38:09Z

@nhodges and @Frankweb - as long as you have both signed the CLA then this is fine. The Google Bot always gets confused when a PR contains commits from more than one author.

fhernandezn · 2015-10-22T20:47:49Z

@petebacondarwin Thank you it's good to know that. Do you need another change to this PR?

petebacondarwin · 2015-10-23T10:05:37Z

docs/content/error/orderBy/notarray.ngdoc

+</div>
+```
+
+orderBy must be used with an array so a subset of items can be returned.


orderBy must be used with an array-like value so a subset of items can be returned.

petebacondarwin · 2015-10-23T10:12:37Z

I have added a few minor comments.
Can you also update the documentation comment to make it clear that people can pass in array-like objects and that a non-array like object will result in an error?
Finally, since this is a breaking change, we need a BREAKING CHANGES block in one of the commits, if you could provide one.
We can squash all these commits together when we merge. It is likely to go into one commit. Can @nhodges and @Frankweb agree on who would be the final author of this?

nhodges · 2015-10-23T17:17:24Z

@petebacondarwin @Frankweb

I went ahead and took care of the requested changes to docs and added a test that takes in a strong and returns a sorted array.

fhernandezn · 2015-10-23T18:04:44Z

@nhodges Good! Please squash the commits, this is your pull request.

gkalpak · 2015-10-27T10:51:29Z

docs/content/error/orderBy/notarray.ngdoc

+  Order by:
+  <select ng-model="orderProp" ng-options="prop for prop in ['id', 'name']"></select>
+</label>
+<div ng-repeat="(key, value) in myObj | orderBy:orderProp">


myObj --> arrFromMyObj

gkalpak · 2015-10-27T10:56:59Z

With #11719 (comment), I think we are good to go 😄
(I left couple of minor comments (typos), but I will take care of them while merging.)

Thx @nhodges and @Frankweb (and @petebacondarwin) for working on this one !

gkalpak · 2015-10-27T12:22:47Z

@nhodges, actually could you rebase your changes on master. It's been a long time since the first commit...

…object. angular#11255

BREAKING CHANGES: These changes alter the behavior of AngularJS such that it will no longer throw an error for non array values, as it will accept array-like values as well. It will throw an error if no array-like value is passed.

nhodges · 2015-10-28T19:24:41Z

@gkalpak rebased. LMK if I can do anything else or if you want me to take care of the changes above.

gkalpak · 2015-10-28T20:41:46Z

src/ng/filter/orderBy.js

@@ -177,7 +177,7 @@ orderByFilter.$inject = ['$parse'];
 function orderByFilter($parse) {
  return function(array, sortPredicate, reverseOrder) {

-    if (!(isArrayLike(array))) return array;
+    if (!(isArrayLike(array))) throw minErr('orderBy')('notarray', 'Expected array but received: {0}', array);


I was in the middle of merging when I realized there is an issue here: We need to allow null/undefined (in order to support asnc loading).
This block needs to be modified (see filter#L132-L138 for an example).

@gkalpak I guess something like

if(!array) return array; if (!(isArrayLike(array))) throw minErr('orderBy')('notarray', 'Expected array but received: {0}', array);

would be easier to read than the example.

This is just an idea,of course :) what do you think about that?

I think that is about right and similar to what we do in other filters.

In filterFilter we only allow null, undefined and array-like (and I think that should we do here as well). So it would be more like:

if (array == null) return array;

gkalpak · 2015-10-28T20:43:29Z

@nhodges, there seems to be one last little thing to do 😄
Could you take care of #11719 (comment) and add a test that verifies null/undefined do not result in an error ?

nhodges · 2015-10-29T16:43:22Z

@gkalpak Sure thing, please take a look at the latest changeset. Cheers!

gkalpak · 2015-10-31T00:55:22Z

I updated the documentation (per #11719 (comment)), made the change described in #11719 (comment), fixed some typos in the error message and merged !

Well done everyone 😃

BREAKING CHANGE: Previously, an non array-like input would pass through the orderBy filter unchanged. Now, an error is thrown. This can be worked around by converting an object to an array, either manually or using a filter such as https://github.com/petebacondarwin/angular-toArrayFilter. (`null` and `undefined` still pass through without an error, in order to support asynchronous loading of resources.) Closes angular#11255 Closes angular#11719

googlebot added the cla: yes label Apr 24, 2015

petebacondarwin added this to the 1.5.x - migration-facilitation milestone Jun 30, 2015

petebacondarwin reviewed Jun 30, 2015
View reviewed changes

googlebot added cla: no and removed cla: yes labels Oct 13, 2015

fhernandezn reviewed Oct 13, 2015
View reviewed changes

gkalpak reviewed Oct 15, 2015
View reviewed changes

petebacondarwin reviewed Oct 23, 2015
View reviewed changes

nhodges force-pushed the dev/nh/11255 branch from 7236c62 to a65d3a4 Compare October 23, 2015 16:49

gkalpak reviewed Oct 27, 2015
View reviewed changes

nhodges and others added 5 commits October 28, 2015 10:21

fix(orderBy): Throw exception when orderBy is given a non array-like …

d93e526

…object. angular#11255

Documentation to orderby when the parameter is not an array.

6ddfc27

fix(orderBy): Change filter to orderBy in error thrown.

77cefef

chore(orderBy): Update documentation based on code review.

0c47849

nhodges force-pushed the dev/nh/11255 branch from a65d3a4 to 1c997ce Compare October 28, 2015 17:28

gkalpak reviewed Oct 28, 2015
View reviewed changes

fix(orderBy): Return null/undefined as-is, add specs for this edge case.

e1e87ec

gkalpak closed this in 2a85a63 Oct 31, 2015

fix(orderBy): Throw exception when orderBy is given a non array-like object. #11719

fix(orderBy): Throw exception when orderBy is given a non array-like object. #11719

Conversation

nhodges commented Apr 24, 2015

Choose a reason for hiding this comment

fhernandezn commented Oct 8, 2015

nhodges commented Oct 12, 2015

fhernandezn commented Oct 13, 2015

googlebot commented Oct 13, 2015

nhodges commented Oct 13, 2015

nhodges commented Oct 13, 2015

fhernandezn commented Oct 13, 2015

Choose a reason for hiding this comment

nhodges commented Oct 14, 2015

gkalpak commented Oct 15, 2015

Choose a reason for hiding this comment

nhodges commented Oct 15, 2015

nhodges commented Oct 19, 2015

fhernandezn commented Oct 20, 2015

fhernandezn commented Oct 20, 2015

nhodges commented Oct 20, 2015

fhernandezn commented Oct 20, 2015

fhernandezn commented Oct 20, 2015

fhernandezn commented Oct 20, 2015

nhodges commented Oct 20, 2015

fhernandezn commented Oct 20, 2015

petebacondarwin commented Oct 22, 2015

fhernandezn commented Oct 22, 2015

Choose a reason for hiding this comment

petebacondarwin commented Oct 23, 2015

nhodges commented Oct 23, 2015

fhernandezn commented Oct 23, 2015

Choose a reason for hiding this comment

gkalpak commented Oct 27, 2015

gkalpak commented Oct 27, 2015

nhodges commented Oct 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Oct 28, 2015

nhodges commented Oct 29, 2015

gkalpak commented Oct 31, 2015