Fix default sort comparator to logically handle null/undefined values #922
Conversation
There was a problem hiding this comment.
Nice! This makes sense. Could you also add some test cases to cover this in comparators.test.js? And could you replace some of the firstNames in the table examples' data store with null and undefined so we can manually verify the sorting behavior in the browser?
…lude some empty values
|
@cjcenizal Added the test and updated docs data |
| @@ -46,3 +46,10 @@ describe('comparators - property', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('default comparator', () => { | |||
| test('null/undefined values are sorted to the end', () => { | |||
| const sorted = [undefined, '7', 3, undefined, 5, undefined].sort(Comparators.default()); | |||
There was a problem hiding this comment.
Maybe change one of the undefined values to null to cover both cases?
There was a problem hiding this comment.
I was afraid the undefined nature of sorting null/undefined values would make this unstable, but apparently v8 prepares the array by moving all undefineds to the end of the unsorted array, so it's stable in node to check the null exists first.
|
jenkins test this |
Fixes #920 by checking if sort values are
nullorundefinedand handling those separately. This change does not handle all "invalid" comparisons (e.g.{} > {}is alwaysfalse, and there are other cases) but I don't think the default comparator should incorporate the necessary logic to handle all of the edge cases (Symbol.toPrimitive and OrdinaryToPrimitive).