Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Update(orderBy): Guaranteed stable sort #15914

Closed
wants to merge 1 commit into from
Closed

Update(orderBy): Guaranteed stable sort #15914

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/ng/filter/orderBy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.)
Copy link
Member

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).

* specified predicates nor the custom comparator can distinguish between two items, `orderBy` will
Copy link
Member

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not fully understand this change, this should be just
return defaultCompare(v1.tieBreaker, v2.tieBreaker) * descending;

There is no reason for an external function to understand the internal tieBreaker structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not intended to defer to the defaultCompare function over the user-provided function, but rather to provide a reasonable fallback should a user-provided function fail to the break the tie. See #14481 (comment)

Copy link
Member

Choose a reason for hiding this comment

The 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?
I thought it is useful to give the custom comparator a chance to break the tie in its own way (in the rare case where this will be needed).

}
};

Expand Down
13 changes: 13 additions & 0 deletions test/ng/filter/orderBySpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,19 @@ describe('Filter: orderBy', function() {

expect(orderBy(items, expr, reverse, comparator)).toEqual(sorted);
});

it('should use the default comparator to break ties on a provided comparator', function() {
// Some list that won't be sorted "naturally", i.e. should sort to ['a', 'B', 'c']
var items = ['c', 'a', 'B'];
var expr = null;
function comparator() {
return 0;
}
var reversed = ['B', 'a', 'c'];

expect(orderBy(items, expr, false, comparator)).toEqual(items);
expect(orderBy(items, expr, true, comparator)).toEqual(reversed);
});
});

describe('(object as `value`)', function() {
Expand Down