fix(orderBy): ensure correct ordering with arrays of objects and no predicate #12072

petebacondarwin · 2015-06-10T09:22:43Z

This fix also considerably refactors how the orderBy filter works in internally.

The new algorithm precomputes the predicate values for the array being ordered. This makes the algorith easier to follow but also ensures that this computation is done a maximum of n times, whereas in the previous algorithm it could be greater. The downside is that we must temporarily store this intermediate array.

There could be performance implications, but they are as likely to be beneficial as detrimental. It could probably do with benchmarking.

Closes #11866

petebacondarwin · 2015-06-10T09:23:26Z

@Narretz and @wawyed could you take a look?

petebacondarwin · 2015-06-10T09:26:17Z

I guess this should be backported to 1.3.x - but not to 1.2.x since it uses Array.map and Array.reduce, which are not available in IE8.

petebacondarwin · 2015-06-10T09:33:19Z

Here is a plunker with this build of AngularJS demonstrating the original bug is fixed: http://plnkr.co/edit/WxAW671zu7BePcN4wgBn?p=preview

Narretz · 2015-06-10T09:40:35Z

test/ng/filter/orderBySpec.js

-      expect(orderBy([2, 1, 3], [''])).toEqual([1, 2, 3]);
+      // expect(orderBy([2, 1, 3], '')).toEqual([1, 2, 3]);
+      // expect(orderBy([2, 1, 3], [])).toEqual([1, 2, 3]);
+      // expect(orderBy([2, 1, 3], [''])).toEqual([1, 2, 3]);


Is this intentionally commented out?

wawyed · 2015-06-10T09:41:52Z

Seems to be working fine when setting reverse true, although it doesn't seem to be working when using the '-' notation instead of reverse true for array with objects.

Pulnker with the issue:

http://plnkr.co/edit/Z3cdF3wMJ7Uzubw2Gma9?p=preview

petebacondarwin · 2015-06-10T11:30:20Z

Agh! I knew it wasn't going to be so easy.
Now here is the problem. If you are reversing with a predicate then you need a way of telling the algorithm that objects should be ordered by index, rather than identity - but only if they don't have a toString or toValue or are being mapped by specific predicate.

petebacondarwin · 2015-06-10T12:07:06Z

OK, so I fixed it...

Actually, the reason for the refactoring was that it made this scenario much easier to handle.

But I am not 100% happy that we have to guess at whether the object is a POJO or not, when we decide to use the index rather than the identity for its ordering value.

Also, we now treat null as lower order than an object - hence the change to two of the tests. I don't think this should really affect anyone but perhaps it should be marked as a potential breaking change?

petebacondarwin · 2015-06-10T12:40:56Z

Agh! So safari wasn't happy comparing nulls, so I have now fixed that too

petebacondarwin · 2015-06-10T12:58:15Z

http://plnkr.co/edit/BSkUBBUfg0c9TJ9tTK5F?p=preview

wawyed · 2015-06-10T16:12:33Z

For example in this plunkr is this behaviour desired for the third ng-repeat?:

http://plnkr.co/edit/gyw8B10H9tpGdu77g1e7?p=preview

According to documentation if reverse is true it should reverse the order of the array, no matter what type the contents are. It's not that clear what it should do with '-'.

EDIT:

Unless we assume that orderBy will always order the array in ascending order unless it's a POJO where it would order by index. But personally I would rather make it order by index if it's object like and ascending if it's a string/number.

lgalfaso · 2015-06-10T18:30:35Z

src/ng/filter/orderBy.js

    if (!(isArrayLike(array))) return array;
-    sortPredicate = isArray(sortPredicate) ? sortPredicate : [sortPredicate];
+
+    if (!isArray(sortPredicate)) { sortPredicate = [sortPredicate]; }
    if (sortPredicate.length === 0) { sortPredicate = ['+']; }


Should this be

sortPredicate.push('+');

?

Mmm... thinking a little more about this, I think my proposal and the current code are wrong as we are modifying the original sortPredicate and we should not do that

I agree. We should not modify the passed in predicate

How is the current implementation modifying the passed in predicate ?

It's not. But I agree that we should start doing that either :-)

lgalfaso · 2015-06-10T18:45:03Z

Also, we now treat null as lower order than an object - hence the change to two of the tests. I don't think this should really affect anyone but perhaps it should be marked as a potential breaking change?

Do you think it would make the code any harder to keep the original behavior? I do not mind this going one way or another, but do think this is a small breaking change

hamfastgamgee · 2015-06-12T01:59:52Z

I'm guessing that this "small breaking change" might be accidentally fixing (or at least changing behavior of) #11312?

petebacondarwin · 2015-06-16T21:26:38Z

I think we can probably move null in the sort order. I need to work out exactly where that is though.

petebacondarwin · 2015-06-16T22:52:15Z

No breaking change no more: the algorithm sorts nulls in the same way as array.sort().
Objects are now dealt with in the following manner:

has a valid toValue() method: sort on the return value of this method
has a valid custom toString() method (i.e. not Object.prototype.toString()): sort on the return value of this method
otherwise sort on index of the object in the array

This means that dates are sorted as expected, but also that arrays are sorted by their toString representation, which is to convert each item to a string and then join them together, separated by commas. This gives the same result as calling array.sort() on such an array of arrays.

I have noted that the idiom used is a Schwartzian Transform and that it closes #4282.

lgalfaso · 2015-06-17T06:55:58Z

src/ng/filter/orderBy.js

+
+    function doComparison(v1, v2) {
+      var result = 0;
+      for (var index=0, length = predicates.length; index < length; ++index) {


for (...; !result && index < length; ...) {

?

But it is a bit obscure. I would prefer to make it more explicit:

function doComparison(v1, v2) { var result = 0; for (var index=0, length = predicates.length; index < length; ++index) { result = compare(v1.predicateValues[index], v2.predicateValues[index]) * predicates[index].descending; if (result) break; } return result; }

Works for me

petebacondarwin · 2015-06-17T12:46:37Z

@lgalfaso I updated the code.

lgalfaso · 2015-06-17T19:06:45Z

LGTM

…reused

…redicate By refactoring to use a Schwartzian transform, we can ensure that objects with no custom `toString` or `toValue` methods are just ordered using their position in the original collection. Closes angular#11866 Closes angular#11312 Closes angular#4282

petebacondarwin added type: bug frequency: low cla: yes needs: review severity: broken expected use component: filters branch: 1.4.x labels Jun 10, 2015

petebacondarwin added this to the 1.4.1 milestone Jun 10, 2015

petebacondarwin assigned Narretz Jun 10, 2015

Narretz reviewed Jun 10, 2015
View reviewed changes

petebacondarwin force-pushed the issue-11866 branch from 235849f to 7020ecc Compare June 10, 2015 11:31

lgalfaso reviewed Jun 10, 2015
View reviewed changes

petebacondarwin assigned petebacondarwin and unassigned Narretz Jun 11, 2015

lgalfaso mentioned this pull request Jun 14, 2015

feat(orderBy): speed optimization #4282

Closed

petebacondarwin removed this from the 1.4.1 milestone Jun 16, 2015

petebacondarwin modified the milestones: 1.4.2, 1.4.1 Jun 16, 2015

petebacondarwin force-pushed the issue-11866 branch 2 times, most recently from cafc829 to 5fb4e14 Compare June 16, 2015 22:44

lgalfaso reviewed Jun 17, 2015
View reviewed changes

petebacondarwin added 2 commits June 18, 2015 08:33

refact(filter): move hasCustomToString into shared namespace to be …

c5a3d8f

…reused

petebacondarwin force-pushed the issue-11866 branch from d317e1e to afdd1df Compare June 18, 2015 07:33

petebacondarwin merged commit afdd1df into angular:master Jun 18, 2015

petebacondarwin deleted the issue-11866 branch November 24, 2016 09:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(orderBy): ensure correct ordering with arrays of objects and no predicate #12072

fix(orderBy): ensure correct ordering with arrays of objects and no predicate #12072

petebacondarwin commented Jun 10, 2015

petebacondarwin commented Jun 10, 2015

petebacondarwin commented Jun 10, 2015

petebacondarwin commented Jun 10, 2015

Narretz Jun 10, 2015

petebacondarwin Jun 10, 2015

wawyed commented Jun 10, 2015

petebacondarwin commented Jun 10, 2015

petebacondarwin commented Jun 10, 2015

petebacondarwin commented Jun 10, 2015

petebacondarwin commented Jun 10, 2015

wawyed commented Jun 10, 2015

lgalfaso Jun 10, 2015

lgalfaso Jun 10, 2015

petebacondarwin Jun 16, 2015

gkalpak Jun 16, 2015

petebacondarwin Jun 16, 2015

lgalfaso commented Jun 10, 2015

hamfastgamgee commented Jun 12, 2015

petebacondarwin commented Jun 16, 2015

petebacondarwin commented Jun 16, 2015

lgalfaso Jun 17, 2015

petebacondarwin Jun 17, 2015

petebacondarwin Jun 17, 2015

lgalfaso Jun 17, 2015

petebacondarwin commented Jun 17, 2015

lgalfaso commented Jun 17, 2015

fix(orderBy): ensure correct ordering with arrays of objects and no predicate #12072

fix(orderBy): ensure correct ordering with arrays of objects and no predicate #12072

Conversation

petebacondarwin commented Jun 10, 2015

petebacondarwin commented Jun 10, 2015

petebacondarwin commented Jun 10, 2015

petebacondarwin commented Jun 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wawyed commented Jun 10, 2015

petebacondarwin commented Jun 10, 2015

petebacondarwin commented Jun 10, 2015

petebacondarwin commented Jun 10, 2015

petebacondarwin commented Jun 10, 2015

wawyed commented Jun 10, 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

Choose a reason for hiding this comment

lgalfaso commented Jun 10, 2015

hamfastgamgee commented Jun 12, 2015

petebacondarwin commented Jun 16, 2015

petebacondarwin commented Jun 16, 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

petebacondarwin commented Jun 17, 2015

lgalfaso commented Jun 17, 2015