Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(collection)!: default sort comparison algorithm #10412

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
27 changes: 27 additions & 0 deletions packages/collection/__tests__/collection.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -813,6 +813,33 @@ describe('sort() tests', () => {
const coll = createCollectionFrom(['a', 5], ['b', 3], ['c', 1]);
expect(coll.sort()).toStrictEqual(createCollectionFrom(['c', 1], ['b', 3], ['a', 5]));
});

describe('returns correct sort order for test values', () => {
const defaultSort = Collection['defaultSort']; // eslint-disable-line @typescript-eslint/dot-notation
const testDefaultSortOrder = (firstValue: any, secondValue: any, result: number) => {
expect(defaultSort(firstValue, secondValue)).toStrictEqual(result);
expect(defaultSort(secondValue, firstValue)).toStrictEqual(result ? result * -1 : 0);
};

test('correctly evaluates sort order of undefined', () => {
testDefaultSortOrder(undefined, undefined, 0);
testDefaultSortOrder(0, undefined, -1);
});

test('correctly evaluates numeric values stringwise', () => {
testDefaultSortOrder(-1, -2, -1); // "-1" before "-2"
testDefaultSortOrder(1, '1', 0); // "1" equal to "1"
testDefaultSortOrder(1, '1.0', -1); // "1" before "1.0"
testDefaultSortOrder(1.1, '1.1', 0); // "1.1" equal to "1.1"
testDefaultSortOrder('01', 1, -1); // "01" before "1"
testDefaultSortOrder(1, 1n, 0); // "1" equal to "1"
testDefaultSortOrder(Number.NaN, 'NaN', 0); // "NaN" equal to "NaN"
});

test('evaluates object literals as equal', () => {
testDefaultSortOrder({ a: 1 }, { b: 2 }, 0);
});
});
});
});

Expand Down
33 changes: 23 additions & 10 deletions packages/collection/src/collection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -792,10 +792,11 @@ export class Collection<Key, Value> extends Map<Key, Value> {
/**
* The sort method sorts the items of a collection in place and returns it.
* The sort is not necessarily stable in Node 10 or older.
* The default sort order is according to string Unicode code points.
* If a comparison function is not provided, the function sorts by element values, using the same stringwise comparison algorithm as
* {@link https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort | Array.sort()}.
*
* @param compareFunction - Specifies a function that defines the sort order.
* If omitted, the collection is sorted according to each character's Unicode code point value, according to the string conversion of each element.
* @param compareFunction - Specifies a function that defines the sort order. The return value of this function should be negative if
* `a` comes before `b`, positive if `b` comes before `a`, or zero if `a` and `b` are considered equal.
* @example
* ```ts
* collection.sort((userA, userB) => userA.createdTimestamp - userB.createdTimestamp);
Expand Down Expand Up @@ -983,16 +984,16 @@ export class Collection<Key, Value> extends Map<Key, Value> {
}

/**
* The sorted method sorts the items of a collection and returns it.
* The toSorted method returns a shallow copy of the collection with the items sorted.
* The sort is not necessarily stable in Node 10 or older.
* The default sort order is according to string Unicode code points.
* If a comparison function is not provided, the function sorts by element values, using the same stringwise comparison algorithm as
* {@link https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort | Array.sort()}.
*
* @param compareFunction - Specifies a function that defines the sort order.
* If omitted, the collection is sorted according to each character's Unicode code point value,
* according to the string conversion of each element.
* @param compareFunction - Specifies a function that defines the sort order. The return value of this function should be negative if
* `a` comes before `b`, positive if `b` comes before `a`, or zero if `a` and `b` are considered equal.
* @example
* ```ts
* collection.sorted((userA, userB) => userA.createdTimestamp - userB.createdTimestamp);
* const sortedCollection = collection.toSorted((userA, userB) => userA.createdTimestamp - userB.createdTimestamp);
* ```
*/
public toSorted(compareFunction: Comparator<Key, Value> = Collection.defaultSort) {
Expand All @@ -1004,8 +1005,20 @@ export class Collection<Key, Value> extends Map<Key, Value> {
return [...this.entries()];
}

/**
* Emulates the default sort comparison algorithm used in ECMAScript. Equivalent to calling the
* {@link https://tc39.es/ecma262/multipage/indexed-collections.html#sec-comparearrayelements | CompareArrayElements}
* operation with arguments `firstValue`, `secondValue` and `undefined`.
*/
private static defaultSort<Value>(firstValue: Value, secondValue: Value): number {
return Number(firstValue > secondValue) || Number(firstValue === secondValue) - 1;
if (firstValue === undefined) return secondValue === undefined ? 0 : 1;
if (secondValue === undefined) return -1;

const x = String(firstValue);
const y = String(secondValue);
Renegade334 marked this conversation as resolved.
Show resolved Hide resolved
if (x < y) return -1;
if (y < x) return 1;
return 0;
}

/**
Expand Down