-
Notifications
You must be signed in to change notification settings - Fork 27.4k
Update(orderBy): Guaranteed stable sort #15914
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,9 +49,8 @@ | |
* second, or `1` otherwise. | ||
* | ||
* In order to ensure that the sorting will be deterministic across platforms, if none of the | ||
* specified predicates can distinguish between two items, `orderBy` will automatically introduce a | ||
* dummy predicate that returns the item's index as `value`. | ||
* (If you are using a custom comparator, make sure it can handle this predicate as well.) | ||
* specified predicates nor the custom comparator can distinguish between two items, `orderBy` will | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is confusing imo. The custom comparator is not something additional to the predicates. They are used together (the predicates provide the values and the comparator compares them). |
||
* automatically introduce a dummy predicate that returns the item's index as `value`. | ||
* | ||
* Finally, in an attempt to simplify things, if a predicate returns an object as the extracted | ||
* value for an item, `orderBy` will try to convert that object to a primitive value, before passing | ||
|
@@ -599,7 +598,7 @@ function orderByFilter($parse) { | |
} | ||
} | ||
|
||
return compare(v1.tieBreaker, v2.tieBreaker) * descending; | ||
return (compare(v1.tieBreaker, v2.tieBreaker) || defaultCompare(v1.tieBreaker, v2.tieBreaker)) * descending; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not fully understand this change, this should be just There is no reason for an external function to understand the internal There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not intended to defer to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lgalfaso, for better or worse, this tiebreaker structure is documented and has been used with custom comparator 😕 Do you think we should change that? |
||
} | ||
}; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should not be removed. It is still necessary for the comparator to handle the tie-break value. E.g. if it has an array of strings, it should not expect that all values will be strings, because there is the possibility of getting passed a number (the index).