Commit 0b272c0b authored by Robert Knight's avatar Robert Knight

Add additional tests and clarify string vs number step comparisons

CFI steps that cannot be parsed as base-10 integers are invalid, but we try to
fail gracefully in `compareCFIs` by using a string comparison.  This introduces
a problem when one step is an integer and another is a string. Handle this by
arbitrarily deciding that numbers sort before strings. We could have chosen
other approaches as well.
parent 815cef3c
/**
* Functions for working with EPUB Canonical Fragment Identifiers.
*
* See https://idpf.org/epub/linking/cfi/.
*/
/** /**
* Compare two arrays. * Compare two arrays.
* *
...@@ -14,6 +20,11 @@ function compareArrays( ...@@ -14,6 +20,11 @@ function compareArrays(
for (let i = 0; i < Math.min(a.length, b.length); i++) { for (let i = 0; i < Math.min(a.length, b.length); i++) {
if (a[i] === b[i]) { if (a[i] === b[i]) {
continue; continue;
} else if (typeof a[i] !== typeof b[i]) {
// The result of comparing a number with a string is undefined if the
// string cannot be coerced to a number. To simplify things, we just
// decide that numbers sort before strings.
return typeof a[i] === 'number' ? -1 : 1;
} else if (a[i] < b[i]) { } else if (a[i] < b[i]) {
return -1; return -1;
} else if (a[i] > b[i]) { } else if (a[i] > b[i]) {
...@@ -94,7 +105,10 @@ export function compareCFIs(a: string, b: string): number { ...@@ -94,7 +105,10 @@ export function compareCFIs(a: string, b: string): number {
return stripCFIAssertions(cfi) return stripCFIAssertions(cfi)
.split('/') .split('/')
.map(str => { .map(str => {
const intVal = parseInt(str); // CFI step values _should_ always be integers. We currently handle
// invalid values by using a string comparison instead. We could
// alternatively treat all invalid CFIs as equal.
const intVal = parseInt(str, 10);
return Number.isNaN(intVal) ? str : intVal; return Number.isNaN(intVal) ? str : intVal;
}); });
}; };
......
...@@ -38,6 +38,17 @@ describe('sidebar/util/cfi', () => { ...@@ -38,6 +38,17 @@ describe('sidebar/util/cfi', () => {
b: '/2/4', b: '/2/4',
expected: 0, expected: 0,
}, },
// CFIs of unequal length
{
a: '/2/4/8',
b: '/2/4',
expected: 1,
},
{
a: '/2/4',
b: '/2/4/8',
expected: -1,
},
// Check numeric steps are treated as numbers and not as strings. // Check numeric steps are treated as numbers and not as strings.
{ {
a: '/2/3', a: '/2/3',
...@@ -69,6 +80,12 @@ describe('sidebar/util/cfi', () => { ...@@ -69,6 +80,12 @@ describe('sidebar/util/cfi', () => {
b: '/2/4/abc', b: '/2/4/abc',
expected: 1, expected: 1,
}, },
// Number steps sort before string steps.
{
a: '/2/4',
b: '/2/-',
expected: -1,
},
// Empty CFIs // Empty CFIs
{ {
a: '', a: '',
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment