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 default sort comparator to logically handle null/undefined values #922

Merged
Show file tree
Hide file tree
Changes from 3 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
**Bug fixes**

- Added `role="dialog"` to `EuiFlyout` to improve screen reader accessibility ([#916](https://github.com/elastic/eui/pull/916))
- Default sort comparator (used by `EuiInMemoryTable`) now handles `null` and `undefined` values ([#922](https://github.com/elastic/eui/pull/922))

## [`0.0.52`](https://github.com/elastic/eui/tree/v0.0.52)

Expand Down
2 changes: 1 addition & 1 deletion src-docs/src/views/tables/data_store.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ const createCountries = () => [
];

const firstNames = ['Very long first name that will wrap or be truncated', 'Another very long first name which will wrap or be truncated',
'Clinton', 'Igor', 'Karl', 'Drew', 'Honza', 'Rashid', 'Jordan', 'John'];
'Clinton', 'Igor', undefined, 'Drew', null, 'Rashid', undefined, 'John'];

const lastNames = ['Very long last name that will wrap or be truncated', 'Another very long last name which will wrap or be truncated',
'Gormley', 'Motov', 'Minarik', 'Raines', 'Král', 'Khan', 'Sissel', 'Dorlus'];
Expand Down
28 changes: 28 additions & 0 deletions src/services/sort/comparators.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,34 @@ export const Comparators = Object.freeze({

default: (direction = SortDirection.ASC) => {
return (v1, v2) => {
// JavaScript's comparison of null/undefined (and some others not handled here) values always returns `false`
// (https://www.ecma-international.org/ecma-262/#sec-abstract-relational-comparison)
// resulting in cases where v1 < v2 and v1 > v2 are both false.
// This leads to unpredictable sorting results in multiple JS engine sorting algorithms
// which causes unpredictable user experiences.
// Instead:
// * 1. push undefined/null values to the end of the sorted list, _regardless of sort direction_
// (non-sortable values always appear at the end, and sortable values are sorted at the top)
// * 2. report undefined/null values as equal
// * 3. when both values are comparable they are sorted normally

const v1IsComparable = v1 != null;
const v2IsComparable = v2 != null;

// * 1.
if (v1IsComparable && !v2IsComparable) {
return -1;
}
if (!v1IsComparable && v2IsComparable) {
return 1;
}

// * 2.
if (!v1IsComparable && !v2IsComparable) {
return 0;
}

// * 3.
if (v1 === v2) {
return 0;
}
Expand Down
7 changes: 7 additions & 0 deletions src/services/sort/comparators.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe change one of the undefined values to null to cover both cases?

Copy link
Contributor Author

@chandlerprall chandlerprall Jun 13, 2018

Choose a reason for hiding this comment

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

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.

expect(sorted).toEqual([3, 5, '7', undefined, undefined, undefined])
})
});