From 343bd55e7f0f3292c305ace6477e231859ba83a3 Mon Sep 17 00:00:00 2001 From: Kenrick Date: Mon, 9 Sep 2024 11:02:42 +0800 Subject: [PATCH] fix: code review --- common/reviews/api/node-core-library.api.md | 11 +- libraries/node-core-library/src/Sort.ts | 162 +++++++++++------- libraries/node-core-library/src/index.ts | 2 +- .../node-core-library/src/test/Sort.test.ts | 12 +- 4 files changed, 120 insertions(+), 67 deletions(-) diff --git a/common/reviews/api/node-core-library.api.md b/common/reviews/api/node-core-library.api.md index 9ff8037e54a..502e330a150 100644 --- a/common/reviews/api/node-core-library.api.md +++ b/common/reviews/api/node-core-library.api.md @@ -608,6 +608,12 @@ export interface IRunWithRetriesOptions { retryDelayMs?: number; } +// @public +export interface ISortKeysOptions { + compare?: (x: string, y: string) => number; + deep?: boolean; +} + // @public export interface IStringBuilder { append(text: string): void; @@ -831,10 +837,7 @@ export class Sort { static isSorted(collection: Iterable, comparer?: (x: any, y: any) => number): boolean; static isSortedBy(collection: Iterable, keySelector: (element: T) => any, comparer?: (x: any, y: any) => number): boolean; static sortBy(array: T[], keySelector: (element: T) => any, comparer?: (x: any, y: any) => number): void; - static sortKeys> | unknown[]>(object: T, { deep, compare }?: { - deep?: boolean; - compare?: (x: string, y: string) => number; - }): T; + static sortKeys> | unknown[]>(object: T, options?: ISortKeysOptions): T; static sortMapKeys(map: Map, keyComparer?: (x: K, y: K) => number): void; static sortSet(set: Set, comparer?: (x: T, y: T) => number): void; static sortSetBy(set: Set, keySelector: (element: T) => any, keyComparer?: (x: T, y: T) => number): void; diff --git a/libraries/node-core-library/src/Sort.ts b/libraries/node-core-library/src/Sort.ts index 3643b379fd1..fd02c9ec899 100644 --- a/libraries/node-core-library/src/Sort.ts +++ b/libraries/node-core-library/src/Sort.ts @@ -1,6 +1,38 @@ // Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license. // See LICENSE in the project root for license information. +/** + * Options of {@link Sort.sortKeys} + * + * @public + */ +export interface ISortKeysOptions { + /** + * Whether or not to recursively sort keys, both in objects and arrays + * @defaultValue false + */ + deep?: boolean; + /** + * Custom compare function when sorting the keys + * + * @defaultValue Sort.compareByValue + * @param x - Key name + * @param y - Key name + * @returns -1 if `x` is smaller than `y`, 1 if `x` is greater than `y`, or 0 if the values are equal. + */ + compare?: (x: string, y: string) => number; +} + +interface ISortKeysContext { + cache: SortKeysCache; + options: ISortKeysOptions; +} + +type SortKeysCache = WeakMap< + Partial> | unknown[], + Partial> | unknown[] +>; + /** * Operations for sorting collections. * @@ -235,6 +267,9 @@ export class Sort { /** * Sort the keys given in an object * + * @param object - The object to be sorted + * @param options - The options for sort keys + * * @example * * ```ts @@ -243,80 +278,85 @@ export class Sort { */ public static sortKeys> | unknown[]>( object: T, - { deep, compare }: { deep?: boolean; compare?: (x: string, y: string) => number } = { + options: ISortKeysOptions = { deep: false, compare: Sort.compareByValue } ): T { - function isPlainObject(obj: unknown): obj is object { - return obj !== null && typeof obj === 'object'; - } if (!isPlainObject(object) && !Array.isArray(object)) { throw new TypeError(`Expected object or array`); } + const cache: SortKeysCache = new WeakMap(); + const context: ISortKeysContext = { + cache, + options + }; - const cache: WeakMap< - Partial> | unknown[], - Partial> | unknown[] - > = new WeakMap(); + return Array.isArray(object) + ? (innerSortArray(object, context) as T) + : (innerSortKeys(object, context) as T); + } +} +function isPlainObject(obj: unknown): obj is object { + return obj !== null && typeof obj === 'object'; +} - function innerSortArray(arr: unknown[]): unknown[] { - const resultFromCache: undefined | Partial> | unknown[] = cache.get(arr); - if (resultFromCache !== undefined) { - return resultFromCache as unknown[]; - } - const result: unknown[] = []; - cache.set(arr, result); - if (deep) { - result.push( - ...arr.map((entry) => { - if (Array.isArray(entry)) { - return innerSortArray(entry); - } else if (isPlainObject(entry)) { - return innerSortKeys(entry); - } - return entry; - }) - ); - } else { - result.push(...arr); - } +function innerSortArray(arr: unknown[], context: ISortKeysContext): unknown[] { + const resultFromCache: undefined | Partial> | unknown[] = context.cache.get(arr); + if (resultFromCache !== undefined) { + return resultFromCache as unknown[]; + } + const result: unknown[] = []; + context.cache.set(arr, result); + if (context.options.deep) { + result.push( + ...arr.map((entry) => { + if (Array.isArray(entry)) { + return innerSortArray(entry, context); + } else if (isPlainObject(entry)) { + return innerSortKeys(entry, context); + } + return entry; + }) + ); + } else { + result.push(...arr); + } - return result; - } - function innerSortKeys(obj: Partial>): Partial> { - const resultFromCache: undefined | Partial> | unknown[] = cache.get(obj); - if (resultFromCache !== undefined) { - return resultFromCache as Partial>; - } - const result: Partial> = {}; - const keys: string[] = Object.keys(obj).sort(compare); + return result; +} +function innerSortKeys( + obj: Partial>, + context: ISortKeysContext +): Partial> { + const resultFromCache: undefined | Partial> | unknown[] = context.cache.get(obj); + if (resultFromCache !== undefined) { + return resultFromCache as Partial>; + } + const result: Partial> = {}; + const keys: string[] = Object.keys(obj).sort(context.options.compare); - cache.set(obj, result); + context.cache.set(obj, result); - for (const key of keys) { - const value: unknown = obj[key]; - let newValue: unknown; - if (deep) { - if (Array.isArray(value)) { - newValue = innerSortArray(value); - } else if (isPlainObject(value)) { - newValue = innerSortKeys(value); - } else { - newValue = value; - } - } else { - newValue = value; - } - Object.defineProperty(result, key, { - ...Object.getOwnPropertyDescriptor(obj, key), - value: newValue - }); + for (const key of keys) { + const value: unknown = obj[key]; + let newValue: unknown; + if (context.options.deep) { + if (Array.isArray(value)) { + newValue = innerSortArray(value, context); + } else if (isPlainObject(value)) { + newValue = innerSortKeys(value, context); + } else { + newValue = value; } - - return result; + } else { + newValue = value; } - - return Array.isArray(object) ? (innerSortArray(object) as T) : (innerSortKeys(object) as T); + Object.defineProperty(result, key, { + ...Object.getOwnPropertyDescriptor(obj, key), + value: newValue + }); } + + return result; } diff --git a/libraries/node-core-library/src/index.ts b/libraries/node-core-library/src/index.ts index 70396912fbc..19b7ebd8057 100644 --- a/libraries/node-core-library/src/index.ts +++ b/libraries/node-core-library/src/index.ts @@ -93,7 +93,7 @@ export { type IPathFormatConciselyOptions } from './Path'; export { Encoding, Text, NewlineKind, type IReadLinesFromIterableOptions } from './Text'; -export { Sort } from './Sort'; +export { Sort, type ISortKeysOptions } from './Sort'; export { AlreadyExistsBehavior, FileSystem, diff --git a/libraries/node-core-library/src/test/Sort.test.ts b/libraries/node-core-library/src/test/Sort.test.ts index 17cb8124c0e..9b8be981a5c 100644 --- a/libraries/node-core-library/src/test/Sort.test.ts +++ b/libraries/node-core-library/src/test/Sort.test.ts @@ -115,7 +115,12 @@ describe('Sort.sortKeys', () => { } test('sort the keys of an object', () => { - deepEqualInOrder(Sort.sortKeys({ c: 0, a: 0, b: 0 }), { a: 0, b: 0, c: 0 }); + const unsortedObj = { c: 0, a: 0, b: 0 }; + const sortedObj = Sort.sortKeys(unsortedObj); + // Assert that it's not sorted in-place + expect(sortedObj).not.toBe(unsortedObj); + deepEqualInOrder(unsortedObj, { c: 0, a: 0, b: 0 }); + deepEqualInOrder(sortedObj, { a: 0, b: 0, c: 0 }); }); test('custom compare function', () => { @@ -142,6 +147,11 @@ describe('Sort.sortKeys', () => { object.circular = object; const sortedObject = Sort.sortKeys(object, { deep: true }); + // Assert that it's not sorted in-place + expect(sortedObject).not.toBe(object); + expect(Object.keys(object)).toEqual(['z', 'circular']); + + // Assert that circular value references the same thing expect(sortedObject).toBe(sortedObject.circular); expect(Object.keys(sortedObject)).toEqual(['circular', 'z']);