From 27b483e9454670be4e6301ebdd5d00d45e5cf100 Mon Sep 17 00:00:00 2001 From: Dayong Lee Date: Sat, 31 Aug 2024 14:31:48 +0900 Subject: [PATCH] fix(compat/orderby): add missed test cases and fix the problematic parts (#427) * Delete getPath * Add testcase and fix orderBy * Fix jsdoc * Fix jsdoc * Fix comment * Change variable name * Improve performance * Remove unused * Remove memoize by change logic * Remove unusable type * Add testcase for coverage * Refactor code * Extract condition * Use object only needed * Change variable name and integrate map * Apply suggestions from code review --------- Co-authored-by: Sojin Park --- benchmarks/performance/orderBy.bench.ts | 22 ++-- src/compat/_internal/compareValues.spec.ts | 25 +++++ src/compat/_internal/compareValues.ts | 11 ++ src/compat/_internal/getPath.spec.ts | 24 ----- src/compat/_internal/getPath.ts | 44 -------- src/compat/array/orderBy.spec.ts | 115 +++++++++++++++++++-- src/compat/array/orderBy.ts | 95 ++++++++++------- 7 files changed, 214 insertions(+), 122 deletions(-) create mode 100644 src/compat/_internal/compareValues.spec.ts create mode 100644 src/compat/_internal/compareValues.ts delete mode 100644 src/compat/_internal/getPath.spec.ts delete mode 100644 src/compat/_internal/getPath.ts diff --git a/benchmarks/performance/orderBy.bench.ts b/benchmarks/performance/orderBy.bench.ts index 59cbd53f..b06d2044 100644 --- a/benchmarks/performance/orderBy.bench.ts +++ b/benchmarks/performance/orderBy.bench.ts @@ -9,41 +9,39 @@ const users = [ { user: 'fred', age: 40, nested: { user: 'fred' } }, { user: 'barney', age: 36, nested: { user: 'bar' } }, ]; -const keys: Array = ['user', 'age']; -const orders: Array<'asc' | 'desc'> = ['asc', 'desc']; describe('orderBy', () => { bench('es-toolkit/orderBy', () => { - orderByToolkit(users, keys, orders); - orderByToolkit(users, [user => user.user, user => user.age], orders); + orderByToolkit(users, ['user', 'age'], ['asc', 'desc']); + orderByToolkit(users, [user => user.user, user => user.age], ['asc', 'desc']); }); bench('es-toolkit/compat/orderBy', () => { - orderByToolkitCompat(users, keys, orders); - orderByToolkitCompat(users, [user => user.user, user => user.age], orders); + orderByToolkitCompat(users, ['user', 'age'], ['asc', 'desc']); + orderByToolkitCompat(users, [user => user.user, user => user.age], ['asc', 'desc']); }); bench('lodash/orderBy', () => { - orderByLodash(users, keys, orders); - orderByLodash(users, [user => user.user, user => user.age], orders); + orderByLodash(users, ['user', 'age'], ['asc', 'desc']); + orderByLodash(users, [user => user.user, user => user.age], ['asc', 'desc']); }); }); describe('orderBy (nested property names)', () => { bench('es-toolkit/compat/orderBy', () => { - orderByToolkitCompat(users, [['nested', 'user'], ['age']], orders); + orderByToolkitCompat(users, [['nested', 'user'], ['age']], ['asc', 'desc']); }); bench('lodash/orderBy', () => { - orderByLodash(users, [['nested', 'user'], ['age']], orders); + orderByLodash(users, [['nested', 'user'], ['age']], ['asc', 'desc']); }); }); describe('orderBy (property path)', () => { bench('es-toolkit/compat/orderBy', () => { - orderByToolkitCompat(users, ['nested.user', 'age'], orders); + orderByToolkitCompat(users, ['nested.user', 'age'], ['asc', 'desc']); }); bench('lodash/orderBy', () => { - orderByLodash(users, ['nested.user', 'age'], orders); + orderByLodash(users, ['nested.user', 'age'], ['asc', 'desc']); }); }); diff --git a/src/compat/_internal/compareValues.spec.ts b/src/compat/_internal/compareValues.spec.ts new file mode 100644 index 00000000..8ad50753 --- /dev/null +++ b/src/compat/_internal/compareValues.spec.ts @@ -0,0 +1,25 @@ +import { describe, it, expect } from 'vitest'; +import { compareValues } from './compareValues'; + +describe('compareValues', () => { + it('should return -1', () => { + expect(compareValues(1, 2, 'asc')).toBe(-1); + expect(compareValues(2, 1, 'desc')).toBe(-1); + expect(compareValues(null, 1, 'asc')).toBe(-1); + expect(compareValues(1, null, 'asc')).toBe(-1); + }); + + it('should return 1', () => { + expect(compareValues(2, 1, 'asc')).toBe(1); + expect(compareValues(1, 2, 'desc')).toBe(1); + expect(compareValues(1, null, 'desc')).toBe(1); + expect(compareValues(null, 1, 'desc')).toBe(1); + }); + + it('should return 0', () => { + expect(compareValues(1, 1, 'asc')).toBe(0); + expect(compareValues(1, 1, 'desc')).toBe(0); + expect(compareValues(null, null, 'asc')).toBe(0); + expect(compareValues(null, null, 'desc')).toBe(0); + }); +}); diff --git a/src/compat/_internal/compareValues.ts b/src/compat/_internal/compareValues.ts new file mode 100644 index 00000000..f069649f --- /dev/null +++ b/src/compat/_internal/compareValues.ts @@ -0,0 +1,11 @@ +export const compareValues = (a: V, b: V, order: string) => { + if ((a != null && b == null) || a < b) { + return order === 'desc' ? 1 : -1; + } + + if ((a == null && b != null) || a > b) { + return order === 'desc' ? -1 : 1; + } + + return 0; +}; diff --git a/src/compat/_internal/getPath.spec.ts b/src/compat/_internal/getPath.spec.ts deleted file mode 100644 index 60e03a97..00000000 --- a/src/compat/_internal/getPath.spec.ts +++ /dev/null @@ -1,24 +0,0 @@ -import { describe, it, expect } from 'vitest'; -import { getPath } from './getPath'; - -describe('getPath function', () => { - it('should return a property name from a string', () => { - expect(getPath('a', { a: 1 })).toBe('a'); - }); - - it('should return an array of property names from a deep path string', () => { - expect(getPath('a.b.c', { a: { b: { c: 1 } } })).toEqual(['a', 'b', 'c']); - }); - - it('should return an array of property names from a string array', () => { - expect(getPath(['a', 'b', 'c'], { a: { b: { c: 1 } } })).toEqual(['a', 'b', 'c']); - }); - - it('should return an array of property names from a string array with a nested string', () => { - expect(getPath(['a', 'b.c', 'd'], { a: { b: { c: { d: 1 } } } })).toEqual(['a', 'b', 'c', 'd']); - }); - - it('should return an array of property names from a deep path like string array', () => { - expect(getPath(['a.b.c', 'd'], { 'a.b.c': { d: 1 } })).toEqual(['a.b.c', 'd']); - }); -}); diff --git a/src/compat/_internal/getPath.ts b/src/compat/_internal/getPath.ts deleted file mode 100644 index e585c666..00000000 --- a/src/compat/_internal/getPath.ts +++ /dev/null @@ -1,44 +0,0 @@ -import { isKey } from './isKey'; -import { toPath } from './toPath'; - -/** - * Get the `path` (property name) from the `key` (property name or property path). - * - * @template T - The type of the object. - * @param {((item: T) => unknown) | string | string[]} key - The `key` (property name or property path or custom key function) to query. - * @param {T} object - The object to query. - * @returns {((item: T) => unknown) | string | string[]} The converted key (only property name or custom key function). - */ -export function getPath( - key: ((item: T) => unknown) | string | string[], - object: T -): ((item: T) => unknown) | string | string[] { - if (Array.isArray(key)) { - const path = []; - let target: object = object; - - for (let i = 0; i < key.length; i++) { - const k = key[i]; - - if (isKey(k, target)) { - target = target[k as keyof typeof target]; - path.push(k); - } else { - const keys = toPath(k); - - for (let i = 0; i < keys.length; i++) { - target = target[keys[i] as keyof typeof target]; - path.push(keys[i]); - } - } - } - - return path; - } - - if (typeof key === 'function' || isKey(key, object)) { - return key; - } - - return toPath(key); -} diff --git a/src/compat/array/orderBy.spec.ts b/src/compat/array/orderBy.spec.ts index 65674794..1a023b27 100644 --- a/src/compat/array/orderBy.spec.ts +++ b/src/compat/array/orderBy.spec.ts @@ -1,8 +1,21 @@ import { describe, expect, it } from 'vitest'; import { orderBy } from './orderBy.ts'; import { falsey } from '../_internal/falsey.ts'; +import { zipObject } from '../../array/zipObject.ts'; +import { partialRight } from '../../function/partialRight.ts'; describe('orderBy', () => { + class Pair { + constructor( + public a: number | undefined, + public b: number, + public c: number + ) { + this.a = a; + this.b = b; + this.c = c; + } + } const objects = [ { a: 'x', b: 3 }, { a: 'y', b: 4 }, @@ -10,6 +23,31 @@ describe('orderBy', () => { { a: 'y', b: 2 }, ]; + const stableArray = [ + new Pair(1, 1, 1), + new Pair(1, 2, 1), + new Pair(1, 1, 1), + new Pair(1, 2, 1), + new Pair(1, 3, 1), + new Pair(1, 4, 1), + new Pair(1, 5, 1), + new Pair(1, 6, 1), + new Pair(2, 1, 2), + new Pair(2, 2, 2), + new Pair(2, 3, 2), + new Pair(2, 4, 2), + new Pair(2, 5, 2), + new Pair(2, 6, 2), + new Pair(undefined, 1, 1), + new Pair(undefined, 2, 1), + new Pair(undefined, 3, 1), + new Pair(undefined, 4, 1), + new Pair(undefined, 5, 1), + new Pair(undefined, 6, 1), + ]; + + const stableObject = zipObject('abcdefghijklmnopqrst'.split(''), stableArray); + const nestedObj = [ { id: '4', address: { zipCode: 4, streetName: 'Beta' } }, { id: '3', address: { zipCode: 3, streetName: 'Alpha' } }, @@ -18,6 +56,56 @@ describe('orderBy', () => { { id: '5', address: { zipCode: 4, streetName: 'Alpha' } }, ]; + it(`should sort multiple properties in ascending order`, () => { + const actual = orderBy(objects, ['a', 'b']); + expect(actual).toEqual([objects[2], objects[0], objects[3], objects[1]]); + }); + + it(`should support iteratees`, () => { + const actual = orderBy(objects, [ + 'a', + function (object) { + return object.b; + }, + ]); + expect(actual).toEqual([objects[2], objects[0], objects[3], objects[1]]); + }); + + it(`should perform a stable sort (test in IE > 8 and V8)`, () => { + expect(orderBy(stableArray, ['a', 'c'])).toEqual(stableArray); + expect(orderBy(stableObject, ['a', 'c'])).toEqual(stableArray); + }); + + it(`should not error on nullish elements`, () => { + let actual; + try { + actual = orderBy([...objects, null, undefined], ['a', 'b']); + } catch { + // do nothing + } + + expect(actual).toEqual([objects[2], objects[0], objects[3], objects[1], null, undefined]); + }); + + it(`should work as an iteratee for methods like \`_.reduce\``, () => { + const objects = [ + { a: 'x', 0: 3 }, + { a: 'y', 0: 4 }, + { a: 'x', 0: 1 }, + { a: 'y', 0: 2 }, + ]; + + expect(['a'].reduce(orderBy, objects)).toEqual([objects[0], objects[2], objects[1], objects[3]]); + expect([0].reduce(orderBy, objects)).toEqual([objects[2], objects[3], objects[0], objects[1]]); + expect([[0]].reduce(orderBy, objects)).toEqual([objects[2], objects[3], objects[0], objects[1]]); + + const wrapped = partialRight(orderBy, 'bogus'); + + expect(['a'].reduce(wrapped, objects)).toEqual([objects[0], objects[2], objects[1], objects[3]]); + expect([0].reduce(wrapped, objects)).toEqual([objects[2], objects[3], objects[0], objects[1]]); + expect([[0]].reduce(wrapped, objects)).toEqual([objects[2], objects[3], objects[0], objects[1]]); + }); + it('should return an empty array when the collection is null or undefined', () => { const actual = [null, undefined].map(value => orderBy(value)); const expected = [[], []]; @@ -81,12 +169,25 @@ describe('orderBy', () => { expect(actual).toEqual(expected); }); - it('should work with `keys` specified as functions', () => { - expect(orderBy(objects, [item => item.a, item => item.b], ['desc', 'asc'])).toEqual([ - objects[3], - objects[1], - objects[2], - objects[0], - ]); + it('should work with `deep` property paths', () => { + const actual = orderBy(nestedObj, ['address.zipCode'], ['asc']); + const expected = [nestedObj[2], nestedObj[3], nestedObj[1], nestedObj[0], nestedObj[4]]; + + expect(actual).toEqual(expected); + }); + + it('should work with nested `deep` property paths when paths length is 1', () => { + const actual = orderBy(nestedObj, [['address.zipCode']], ['asc']); + const expected = [nestedObj[2], nestedObj[3], nestedObj[1], nestedObj[0], nestedObj[4]]; + + expect(actual).toEqual(expected); + }); + + it('shoud work with `deep-like` property paths', () => { + const objects = [{ 'a.b': 1 }, { 'a.b': 2 }, { 'a.b': 3 }, { 'a.b': 4 }]; + const actual = orderBy(objects, ['a.b'], ['desc']); + const expected = [objects[3], objects[2], objects[1], objects[0]]; + + expect(actual).toEqual(expected); }); }); diff --git a/src/compat/array/orderBy.ts b/src/compat/array/orderBy.ts index 13a9f9f6..f9c18ee5 100644 --- a/src/compat/array/orderBy.ts +++ b/src/compat/array/orderBy.ts @@ -1,16 +1,18 @@ -import { getPath } from '../_internal/getPath'; +import { compareValues } from '../_internal/compareValues'; +import { isKey } from '../_internal/isKey'; +import { toPath } from '../_internal/toPath'; /** * Sorts an array of objects based on multiple properties and their corresponding order directions. * - * This function takes an array of objects, an array of keys to sort by, and an array of order directions. + * This function takes an array of objects, an array of criteria to sort by, and an array of order directions. * It returns the sorted array, ordering by each key according to its corresponding direction * ('asc' for ascending or 'desc' for descending). If values for a key are equal,string * it moves to the next key to determine the order. * * @template T - The type of elements in the array. - * @param {T[] | null} collection - The array of objects to be sorted. - * @param {((item: T) => unknown) | string | Array<((item: T) => unknown) | string | string[]>} keys - An array of keys (property names or property paths or custom key functions) to sort by. + * @param { T[] | object | null | undefined} collection - The array of objects to be sorted. + * @param {((item: T) => unknown) | PropertyKey | Array<((item: T) => unknown) | PropertyKey | PropertyKey[]>} criteria - An array of criteria (property names or property paths or custom key functions) to sort by. * @param {unknown | unknown[]} orders - An array of order directions ('asc' for ascending or 'desc' for descending). * @returns {T[]} - The sorted array. * @@ -31,62 +33,85 @@ import { getPath } from '../_internal/getPath'; * // { user: 'fred', age: 40 }, * // ] */ -export function orderBy( - collection: T[] | null | undefined, - keys?: ((item: T) => unknown) | string | Array<((item: T) => unknown) | string | string[]>, +export function orderBy( + collection: T[] | object | null | undefined, + criteria?: ((item: T) => unknown) | PropertyKey | Array<((item: T) => unknown) | PropertyKey | PropertyKey[]>, orders?: unknown | unknown[] ): T[] { if (collection == null) { return []; } - if (!Array.isArray(keys)) { - keys = keys == null ? [] : [keys]; + if (!Array.isArray(collection) && typeof collection === 'object') { + collection = Object.values(collection); + } + + if (!Array.isArray(criteria)) { + criteria = criteria == null ? [] : [criteria]; } if (!Array.isArray(orders)) { orders = orders == null ? [] : [orders]; } - const compareValues = (a: V, b: V, order: string) => { - if (a < b) { - return order === 'desc' ? 1 : -1; // Default is ascending order + const getValueByNestedPath = (object: object, path: PropertyKey[]) => { + let target: object = object; + + for (let i = 0; i < path.length && target != null; i++) { + target = target[path[i] as keyof typeof target]; } - if (a > b) { - return order === 'desc' ? -1 : 1; - } - - return 0; + return target; }; - const getValueByPath = (path: string | ((item: T) => unknown) | string[], obj: T) => { - if (Array.isArray(path)) { - let value: object = obj; - - for (let i = 0; i < path.length; i++) { - value = value[path[i] as keyof typeof value]; - } - - return value; + const getValueByCriterion = ( + criterion: PropertyKey | ((item: T) => unknown) | PropertyKey[] | { key: PropertyKey; path: string[] }, + object: T + ) => { + if (object == null) { + return object; } - if (typeof path === 'function') { - return path(obj); + if (typeof criterion === 'function') { + return criterion(object); } - return obj[path as keyof typeof obj]; + if (Array.isArray(criterion)) { + return getValueByNestedPath(object, criterion); + } + + if (typeof criterion !== 'object') { + return object[criterion as keyof typeof object]; + } + + // Case for possible to be a deep path + if (Object.hasOwn(object, criterion.key)) { + return object[criterion.key as keyof typeof object]; + } + + return getValueByNestedPath(object, criterion.path); }; - keys = keys.map(key => getPath(key, collection[0])); + // Prepare all cases for criteria + const preparedCriteria = criteria.map(criterion => { + // lodash handles a array with one element as a single criterion + if (Array.isArray(criterion) && criterion.length === 1) { + criterion = criterion[0]; + } - return collection.slice().sort((a, b) => { - for (let i = 0; i < keys.length; i++) { - const path = keys[i]; + if (typeof criterion === 'function' || Array.isArray(criterion) || isKey(criterion)) { + return criterion; + } - const valueA = getValueByPath(path, a); - const valueB = getValueByPath(path, b); + // If criterion is not key, it has possibility to be a deep path. So we have to prepare both cases. + return { key: criterion, path: toPath(criterion as string) } as const; + }); + + return (collection as T[]).slice().sort((a, b) => { + for (let i = 0; i < criteria.length; i++) { const order = String((orders as unknown[])[i]); // For Object('desc') case + const valueA = getValueByCriterion(preparedCriteria[i], a); + const valueB = getValueByCriterion(preparedCriteria[i], b); const comparedResult = compareValues(valueA, valueB, order);