orderBy filter has odd results when nulls are compared to Date objects

[hamfastgamgee]

Granted, "odd results" are better than bombing completely, which I think is what used to happen in some cases for orderBy. When you have nulls in the property that is being compared in an orderBy filter and that property is generally a Date (or anything else where .valueOf evaluates to a number but typeof === "object"), the results are effectively unpredictable.

In compare, typeof null evaluates to "object", and typeof also evaluates to "object". Thus, we fall into the t1 === t2 && t1 === "object" block, which calls objectToString() on both values. The null ends up getting converted into a string ("null"), and then the Date ends up getting converted into a number by .valueOf. Back up in compare(), the code goes into the next t1 === t2 block. The values are not equal, so it tries the less than check. "null" < any number always evaluates to false, and so the sort order ends up inconsistent (since the null can be either v1 or v2 depending on the call to compare()). The nulls don't go to either the top or the bottom of the list consistently, regardless of sorting "regular" or "reverse".

I can throw together a plunkr later (too late tonight), but I suspect this is at least enough to get someone thinking. :)

Angular version 1.3.13

[hamfastgamgee]

I strongly suspect the same would be true for, say, Number, but I care about the Date situation, and that's less easy to work around without negative consequences. :)

[hamfastgamgee]

Also, mind you, I'm not sure if there's a general solution to how nulls should sort against all object types or even that whatever gets picked for the default behavior will work for me. I might have to write a custom comparer anyway. But at least the default behavior should probably be well-defined.

[pkozlowski-opensource]

@hamfastgamgee could you please share a plunker with a reproduce scenario? I think that it is already fixed in 1.3.14 or master but a live reproduce scenario would allow us to confirm this.

[hamfastgamgee]

http://embed.plnkr.co/dU2LCUMlqIQ44QgjIdWw/preview

Sorts by default in ascending order, and the header for "The Date" column can be clicked to go to descending order. Where the nulls fall in the list seems to be browser dependent, as I'm getting vastly different results in Firefox and Chrome, neither of which I'd even begin to describe as correct. As I said, "odd". :)

[pkozlowski-opensource]

@hamfastgamgee oh, I see, thnx for the reproduce scenario! Yeh, I agree it is odd, let's do something about it!

[pkozlowski-opensource]

So, look into it a bit more and, funny enough, the current behaviour seems to be "by design". If you see comment in a097aa9 the idea seems to be to keep a comparator's behaviour close to what JS would do. And indeed, as unexpected as it sounds JS will do (at least in Chrome, FFox and Safari):

[null, 'a', null, 'z', null].sort() => ["a", null, null, null, "z"]

So the question here is: do we want to stick to browsers' behaviour or push nulls to the beginning / end? @caitp any particular thoughts on this?

[caitp]

I think orderBy lets you provide custom sort functions (iirc), so custom heuristics should be easy for users to implement

[caitp]

Oh, I guess the function expressions don't really enable custom sorting, so that's kind of dumb

[hamfastgamgee]

What's weird to me is that it doesn't actually seem to act like sort() in my plnkr. If I execute the following at the Chrome console:
[new Date(2015,3,7).valueOf(),new Date(2015,4,5).valueOf(), "null", new Date(2014,10,14).valueOf(), "null", new Date().valueOf(), "null", new Date(2017,10,10).valueOf(),"null"].sort()

It returns with the nulls as strings (which is what objectToString is turning them into) sorted to the end, and all of the date.valueOf values sorted to the beginning (again, as expected from the return value of objectToString).

As far as I can tell, that's exactly what orderBy is intended to be doing, but it's not. The list ends up relatively randomly sorted, and the sort order is different in Chrome and Firefox.

[hamfastgamgee]

It leads me to suspect that Array.prototype.sort is doing something special to account for the null values. Because correct me if I'm wrong,but doesn't "null" < 1 basically get evaluated as NaN < 1?

[hamfastgamgee]

(And, of course, .toString() on all the valueOf calls would potentially hose general date sorting again, since "11"<"3"===true.)

[petebacondarwin]

My PR appears to tidy up this issue: http://plnkr.co/edit/attBdNAiAIKoAXg7X5Ke?p=preview

[hamfastgamgee]

Yup, I can totally live with predictable behavior. I could foresee times when developers would want to define whether null was a "high" or "low" value, but leaving that as "needs to be done in a custom sorter" is good enough for me to consider this issue fixed.

[hamfastgamgee]

i.e. It passes the "I'm in a demo, my product owner asked me to click that sorting column, and nothing weird happened" test. :)

[petebacondarwin]

Great! We'll close this when it lands.