fix(b-table): sort handling for numeric string values (closes #6092) (#6105)

jacobmllr95 · web-flow · commit 29fbcb58c5ef · 2020-11-30T14:04:26.000+01:00
* fix(b-table): sort handling for numeric string values

* Update stringify-object-values.spec.js

* Update stringify-object-values.spec.js
diff --git a/src/components/table/helpers/default-sort-compare.js b/src/components/table/helpers/default-sort-compare.js
@@ -1,37 +1,62 @@
 import get from '../../../utils/get'
-import { isDate, isFunction, isNumber, isUndefinedOrNull } from '../../../utils/inspect'
-import stringifyObjectValues from './stringify-object-values'
+import stringifyObjectValues from '../../../utils/stringify-object-values'
+import { isDate, isFunction, isNumber, isNumeric, isUndefinedOrNull } from '../../../utils/inspect'
+import { toFloat } from '../../../utils/number'
+
+const normalizeValue = value => {
+  if (isUndefinedOrNull(value)) {
+    return ''
+  }
+  if (isNumeric(value)) {
+    return toFloat(value)
+  }
+  return value
+}
 
 // Default sort compare routine
 //
-// TODO: Add option to sort by multiple columns (tri-state per column,
-//       plus order of columns in sort)  where sortBy could be an array
-//       of objects `[ {key: 'foo', sortDir: 'asc'}, {key:'bar', sortDir: 'desc'} ...]`
-//       or an array of arrays `[ ['foo','asc'], ['bar','desc'] ]`
-//       Multisort will most likely be handled in mixin-sort.js by
-//       calling this method for each sortBy
-const defaultSortCompare = (a, b, sortBy, sortDesc, formatter, localeOpts, locale, nullLast) => {
+// TODO:
+//   Add option to sort by multiple columns (tri-state per column,
+//   plus order of columns in sort) where `sortBy` could be an array
+//   of objects `[ {key: 'foo', sortDir: 'asc'}, {key:'bar', sortDir: 'desc'} ...]`
+//   or an array of arrays `[ ['foo','asc'], ['bar','desc'] ]`
+//   Multisort will most likely be handled in `mixin-sort.js` by
+//   calling this method for each sortBy
+const defaultSortCompare = (
+  a,
+  b,
+  { sortBy = null, formatter = null, locale = undefined, localeOptions = {}, nullLast = false } = {}
+) => {
+  // Get the value by `sortBy`
   let aa = get(a, sortBy, null)
   let bb = get(b, sortBy, null)
+
+  // Apply user-provided formatter
   if (isFunction(formatter)) {
     aa = formatter(aa, sortBy, a)
     bb = formatter(bb, sortBy, b)
   }
-  aa = isUndefinedOrNull(aa) ? '' : aa
-  bb = isUndefinedOrNull(bb) ? '' : bb
+
+  // Internally normalize value
+  // `null` / `undefined` => ''
+  // `'0'` => `0`
+  aa = normalizeValue(aa)
+  bb = normalizeValue(bb)
+
   if ((isDate(aa) && isDate(bb)) || (isNumber(aa) && isNumber(bb))) {
     // Special case for comparing dates and numbers
     // Internally dates are compared via their epoch number values
     return aa < bb ? -1 : aa > bb ? 1 : 0
   } else if (nullLast && aa === '' && bb !== '') {
-    // Special case when sorting null/undefined/empty string last
+    // Special case when sorting `null` / `undefined` / '' last
     return 1
   } else if (nullLast && aa !== '' && bb === '') {
-    // Special case when sorting null/undefined/empty string last
+    // Special case when sorting `null` / `undefined` / '' last
     return -1
   }
+
   // Do localized string comparison
-  return stringifyObjectValues(aa).localeCompare(stringifyObjectValues(bb), locale, localeOpts)
+  return stringifyObjectValues(aa).localeCompare(stringifyObjectValues(bb), locale, localeOptions)
 }
 
 export default defaultSortCompare
diff --git a/src/components/table/helpers/default-sort-compare.spec.js b/src/components/table/helpers/default-sort-compare.spec.js
@@ -2,47 +2,53 @@ import defaultSortCompare from './default-sort-compare'
 
 describe('table/helpers/default-sort-compare', () => {
   it('sorts numbers correctly', async () => {
-    expect(defaultSortCompare({ a: 1 }, { a: 2 }, 'a')).toBe(-1)
-    expect(defaultSortCompare({ a: 2 }, { a: 1 }, 'a')).toBe(1)
-    expect(defaultSortCompare({ a: 1 }, { a: 1 }, 'a')).toBe(0)
-    expect(defaultSortCompare({ a: -1 }, { a: 1 }, 'a')).toBe(-1)
-    expect(defaultSortCompare({ a: 1 }, { a: -1 }, 'a')).toBe(1)
-    expect(defaultSortCompare({ a: 0 }, { a: 0 }, 'a')).toBe(0)
-    expect(defaultSortCompare({ a: 1.234 }, { a: 1.567 }, 'a')).toBe(-1)
-    expect(defaultSortCompare({ a: 1.561 }, { a: 1.234 }, 'a')).toBe(1)
+    const options = { sortBy: 'a' }
+    expect(defaultSortCompare({ a: 1 }, { a: 2 }, options)).toBe(-1)
+    expect(defaultSortCompare({ a: 2 }, { a: 1 }, options)).toBe(1)
+    expect(defaultSortCompare({ a: 1 }, { a: 1 }, options)).toBe(0)
+    expect(defaultSortCompare({ a: -1 }, { a: 1 }, options)).toBe(-1)
+    expect(defaultSortCompare({ a: 1 }, { a: -1 }, options)).toBe(1)
+    expect(defaultSortCompare({ a: 0 }, { a: 0 }, options)).toBe(0)
+    expect(defaultSortCompare({ a: 1.234 }, { a: 1.567 }, options)).toBe(-1)
+    expect(defaultSortCompare({ a: 1.561 }, { a: 1.234 }, options)).toBe(1)
   })
 
   it('sorts dates correctly', async () => {
     const date1 = { a: new Date(2020, 1, 1) }
     const date2 = { a: new Date(1999, 11, 31) }
     const date3 = { a: new Date(1999, 1, 1) }
     const date4 = { a: new Date(1999, 1, 1, 12, 12, 12, 12) }
+    const options = { sortBy: 'a' }
 
-    expect(defaultSortCompare(date1, date2, 'a')).toBe(1)
-    expect(defaultSortCompare(date1, date1, 'a')).toBe(0)
-    expect(defaultSortCompare(date2, date1, 'a')).toBe(-1)
-    expect(defaultSortCompare(date2, date3, 'a')).toBe(1)
-    expect(defaultSortCompare(date3, date2, 'a')).toBe(-1)
-    expect(defaultSortCompare(date3, date4, 'a')).toBe(-1)
-    expect(defaultSortCompare(date4, date3, 'a')).toBe(1)
-    expect(defaultSortCompare(date4, date4, 'a')).toBe(0)
+    expect(defaultSortCompare(date1, date2, options)).toBe(1)
+    expect(defaultSortCompare(date1, date1, options)).toBe(0)
+    expect(defaultSortCompare(date2, date1, options)).toBe(-1)
+    expect(defaultSortCompare(date2, date3, options)).toBe(1)
+    expect(defaultSortCompare(date3, date2, options)).toBe(-1)
+    expect(defaultSortCompare(date3, date4, options)).toBe(-1)
+    expect(defaultSortCompare(date4, date3, options)).toBe(1)
+    expect(defaultSortCompare(date4, date4, options)).toBe(0)
   })
 
   it('sorts strings correctly', async () => {
+    const options = { sortBy: 'a' }
+
     // Note: string comparisons are locale based
-    expect(defaultSortCompare({ a: 'a' }, { a: 'b' }, 'a')).toBe(-1)
-    expect(defaultSortCompare({ a: 'b' }, { a: 'a' }, 'a')).toBe(1)
-    expect(defaultSortCompare({ a: 'a' }, { a: 'a' }, 'a')).toBe(0)
-    expect(defaultSortCompare({ a: 'a' }, { a: 'aaa' }, 'a')).toBe(-1)
-    expect(defaultSortCompare({ a: 'aaa' }, { a: 'a' }, 'a')).toBe(1)
+    expect(defaultSortCompare({ a: 'a' }, { a: 'b' }, options)).toBe(-1)
+    expect(defaultSortCompare({ a: 'b' }, { a: 'a' }, options)).toBe(1)
+    expect(defaultSortCompare({ a: 'a' }, { a: 'a' }, options)).toBe(0)
+    expect(defaultSortCompare({ a: 'a' }, { a: 'aaa' }, options)).toBe(-1)
+    expect(defaultSortCompare({ a: 'aaa' }, { a: 'a' }, options)).toBe(1)
   })
 
   it('sorts by nested key correctly', async () => {
+    const options = { sortBy: 'a.b' }
+
     // Note: string comparisons are locale based
-    expect(defaultSortCompare({ a: { b: 'a' } }, { a: { b: 'b' } }, 'a.b')).toBe(-1)
-    expect(defaultSortCompare({ a: { b: 'b' } }, { a: { b: 'a' } }, 'a.b')).toBe(1)
-    expect(defaultSortCompare({ a: { b: 'a' } }, { a: { b: 'a' } }, 'a.b')).toBe(0)
-    expect(defaultSortCompare({ a: { b: 'a' } }, { a: { b: 'aaa' } }, 'a.b')).toBe(-1)
+    expect(defaultSortCompare({ a: { b: 'a' } }, { a: { b: 'b' } }, options)).toBe(-1)
+    expect(defaultSortCompare({ a: { b: 'b' } }, { a: { b: 'a' } }, options)).toBe(1)
+    expect(defaultSortCompare({ a: { b: 'a' } }, { a: { b: 'a' } }, options)).toBe(0)
+    expect(defaultSortCompare({ a: { b: 'a' } }, { a: { b: 'aaa' } }, options)).toBe(-1)
   })
 
   it('sorts using provided formatter correctly', async () => {
@@ -53,34 +59,37 @@ describe('table/helpers/default-sort-compare', () => {
         .reverse()
         .join('')
     }
-    expect(defaultSortCompare({ a: 'ab' }, { a: 'b' }, 'a')).toBe(-1)
-    expect(defaultSortCompare({ a: 'ab' }, { a: 'b' }, 'a', false, formatter)).toBe(1)
+
+    expect(defaultSortCompare({ a: 'ab' }, { a: 'b' }, { sortBy: 'a' })).toBe(-1)
+    expect(defaultSortCompare({ a: 'ab' }, { a: 'b' }, { sortBy: 'a', formatter })).toBe(1)
   })
 
   it('sorts nulls always last when sor-null-lasst is set', async () => {
     const x = { a: 'ab' }
     const y = { a: null }
     const z = {}
     const w = { a: '' }
-    const u = undefined
+    const options = { sortBy: 'a', localeOptions: { numeric: true } }
+    const optionsNullLast = { ...options, nullLast: true }
 
     // Without nullLast set (false)
-    expect(defaultSortCompare(x, y, 'a', u, u, { numeric: true }, u, false)).toBe(1)
-    expect(defaultSortCompare(y, x, 'a', u, u, { numeric: true }, u, false)).toBe(-1)
-    expect(defaultSortCompare(x, z, 'a', u, u, { numeric: true }, u, false)).toBe(1)
-    expect(defaultSortCompare(z, x, 'a', u, u, { numeric: true }, u, false)).toBe(-1)
-    expect(defaultSortCompare(y, z, 'a', u, u, { numeric: true }, u, false)).toBe(0)
-    expect(defaultSortCompare(z, y, 'a', u, u, { numeric: true }, u, false)).toBe(0)
-    expect(defaultSortCompare(x, w, 'a', u, u, { numeric: true }, u, false)).toBe(1)
-    expect(defaultSortCompare(w, x, 'a', u, u, { numeric: true }, u, false)).toBe(-1)
+    expect(defaultSortCompare(x, y, options)).toBe(1)
+    expect(defaultSortCompare(y, x, options)).toBe(-1)
+    expect(defaultSortCompare(x, z, options)).toBe(1)
+    expect(defaultSortCompare(z, x, options)).toBe(-1)
+    expect(defaultSortCompare(y, z, options)).toBe(0)
+    expect(defaultSortCompare(z, y, options)).toBe(0)
+    expect(defaultSortCompare(x, w, options)).toBe(1)
+    expect(defaultSortCompare(w, x, options)).toBe(-1)
+
     // With nullLast set
-    expect(defaultSortCompare(x, y, 'a', u, u, { numeric: true }, u, true)).toBe(-1)
-    expect(defaultSortCompare(y, x, 'a', u, u, { numeric: true }, u, true)).toBe(1)
-    expect(defaultSortCompare(x, z, 'a', u, u, { numeric: true }, u, true)).toBe(-1)
-    expect(defaultSortCompare(z, x, 'a', u, u, { numeric: true }, u, true)).toBe(1)
-    expect(defaultSortCompare(y, z, 'a', u, u, { numeric: true }, u, true)).toBe(0)
-    expect(defaultSortCompare(z, y, 'a', u, u, { numeric: true }, u, true)).toBe(0)
-    expect(defaultSortCompare(x, w, 'a', u, u, { numeric: true }, u, true)).toBe(-1)
-    expect(defaultSortCompare(w, x, 'a', u, u, { numeric: true }, u, true)).toBe(1)
+    expect(defaultSortCompare(x, y, optionsNullLast)).toBe(-1)
+    expect(defaultSortCompare(y, x, optionsNullLast)).toBe(1)
+    expect(defaultSortCompare(x, z, optionsNullLast)).toBe(-1)
+    expect(defaultSortCompare(z, x, optionsNullLast)).toBe(1)
+    expect(defaultSortCompare(y, z, optionsNullLast)).toBe(0)
+    expect(defaultSortCompare(z, y, optionsNullLast)).toBe(0)
+    expect(defaultSortCompare(x, w, optionsNullLast)).toBe(-1)
+    expect(defaultSortCompare(w, x, optionsNullLast)).toBe(1)
   })
 })
diff --git a/src/components/table/helpers/mixin-sorting.js b/src/components/table/helpers/mixin-sorting.js
@@ -38,9 +38,7 @@ export default {
         // Supported localCompare options, see `options` section of:
         // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/localeCompare
         type: Object,
-        default: () => {
-          return { numeric: true }
-        }
+        default: () => ({ numeric: true })
       },
       sortCompareLocale: {
         // String: locale code
@@ -102,17 +100,20 @@ export default {
     isSortable() {
       return this.computedFields.some(f => f.sortable)
     },
+    // Sorts the filtered items and returns a new array of the sorted items
+    // When not sorted, the original items array will be returned
     sortedItems() {
-      // Sorts the filtered items and returns a new array of the sorted items
-      // or the original items array if not sorted.
+      const {
+        localSortBy: sortBy,
+        localSortDesc: sortDesc,
+        sortCompareLocale: locale,
+        sortNullLast: nullLast,
+        sortCompare,
+        localSorting
+      } = this
       const items = (this.filteredItems || this.localItems || []).slice()
-      const sortBy = this.localSortBy
-      const sortDesc = this.localSortDesc
-      const sortCompare = this.sortCompare
-      const localSorting = this.localSorting
-      const sortOptions = { ...this.sortCompareOptions, usage: 'sort' }
-      const sortLocale = this.sortCompareLocale || undefined
-      const nullLast = this.sortNullLast
+      const localeOptions = { ...this.sortCompareOptions, usage: 'sort' }
+
       if (sortBy && localSorting) {
         const field = this.computedFieldsObj[sortBy] || {}
         const sortByFormatted = field.sortByFormatted
@@ -121,31 +122,33 @@ export default {
           : sortByFormatted
             ? this.getFieldFormatter(sortBy)
             : undefined
+
         // `stableSort` returns a new array, and leaves the original array intact
         return stableSort(items, (a, b) => {
           let result = null
+          // Call user provided `sortCompare` routine first
           if (isFunction(sortCompare)) {
-            // Call user provided sortCompare routine
-            result = sortCompare(a, b, sortBy, sortDesc, formatter, sortOptions, sortLocale)
+            // TODO:
+            //   Change the `sortCompare` signature to the one of `defaultSortCompare`
+            //   with the next major version bump
+            result = sortCompare(a, b, sortBy, sortDesc, formatter, localeOptions, locale)
           }
+          // Fallback to built-in `defaultSortCompare` if `sortCompare`
+          // is not defined or returns `null`/`false`
           if (isUndefinedOrNull(result) || result === false) {
-            // Fallback to built-in defaultSortCompare if sortCompare
-            // is not defined or returns null/false
-            result = defaultSortCompare(
-              a,
-              b,
+            result = defaultSortCompare(a, b, {
               sortBy,
-              sortDesc,
               formatter,
-              sortOptions,
-              sortLocale,
+              locale,
+              localeOptions,
               nullLast
-            )
+            })
           }
           // Negate result if sorting in descending order
           return (result || 0) * (sortDesc ? -1 : 1)
         })
       }
+
       return items
     }
   },
diff --git a/src/components/table/helpers/stringify-record-values.js b/src/components/table/helpers/stringify-record-values.js
@@ -1,6 +1,6 @@
 import { isObject } from '../../../utils/inspect'
+import stringifyObjectValues from '../../../utils/stringify-object-values'
 import sanitizeRow from './sanitize-row'
-import stringifyObjectValues from './stringify-object-values'
 
 // Stringifies the values of a record, ignoring any special top level field keys
 // TODO: Add option to stringify `scopedSlot` items
diff --git a/src/components/table/table-sorting.spec.js b/src/components/table/table-sorting.spec.js
@@ -240,7 +240,7 @@ describe('table > sorting', () => {
         sortDesc: false,
         sortCompare: (a, b, sortBy) => {
           // We just use our default sort compare to test passing a function
-          return defaultSortCompare(a, b, sortBy)
+          return defaultSortCompare(a, b, { sortBy })
         }
       }
     })
diff --git a/src/utils/stringify-object-values.js b/src/utils/stringify-object-values.js
@@ -1,6 +1,6 @@
-import { keys } from '../../../utils/object'
-import { isDate, isObject, isUndefinedOrNull } from '../../../utils/inspect'
-import { toString } from '../../../utils/string'
+import { isDate, isObject, isUndefinedOrNull } from './inspect'
+import { keys } from './object'
+import { toString } from './string'
 
 // Recursively stringifies the values of an object, space separated, in an
 // SSR safe deterministic way (keys are sorted before stringification)
@@ -10,24 +10,24 @@ import { toString } from '../../../utils/string'
 //   becomes
 //     'one 3 2 zzz 10 12 11'
 //
-// Primitives (numbers/strings) are returned as-is
-// Null and undefined values are filtered out
+// Strings are returned as-is
+// Numbers get converted to string
+// `null` and `undefined` values are filtered out
 // Dates are converted to their native string format
-const stringifyObjectValues = val => {
-  if (isUndefinedOrNull(val)) {
-    /* istanbul ignore next */
+const stringifyObjectValues = value => {
+  if (isUndefinedOrNull(value)) {
     return ''
   }
   // Arrays are also object, and keys just returns the array indexes
   // Date objects we convert to strings
-  if (isObject(val) && !isDate(val)) {
-    return keys(val)
+  if (isObject(value) && !isDate(value)) {
+    return keys(value)
       .sort() // Sort to prevent SSR issues on pre-rendered sorted tables
-      .filter(v => !isUndefinedOrNull(v)) // Ignore undefined/null values
-      .map(k => stringifyObjectValues(val[k]))
+      .map(k => stringifyObjectValues(value[k]))
+      .filter(v => !!v) // Ignore empty strings
       .join(' ')
   }
-  return toString(val)
+  return toString(value)
 }
 
 export default stringifyObjectValues
diff --git a/src/utils/stringify-object-values.spec.js b/src/utils/stringify-object-values.spec.js

Original file line number	Diff line number	Diff line change
`@@ -240,7 +240,7 @@ describe('table > sorting', () => {`
`240`	`240`	`sortDesc: false,`
`241`	`241`	`sortCompare: (a, b, sortBy) => {`
`242`	`242`	`// We just use our default sort compare to test passing a function`
`243`		`- return defaultSortCompare(a, b, sortBy)`
	`243`	`+ return defaultSortCompare(a, b, { sortBy })`
`244`	`244`	`}`
`245`	`245`	`}`
`246`	`246`	`})`