fix: code review

microsoft · Sep 9, 2024 · 343bd55 · 343bd55
1 parent f068060
commit 343bd55
Show file tree

Hide file tree

Showing 4 changed files with 120 additions and 67 deletions.
diff --git 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<TResult> {
     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<T>(collection: Iterable<T>, comparer?: (x: any, y: any) => number): boolean;
     static isSortedBy<T>(collection: Iterable<T>, keySelector: (element: T) => any, comparer?: (x: any, y: any) => number): boolean;
     static sortBy<T>(array: T[], keySelector: (element: T) => any, comparer?: (x: any, y: any) => number): void;
-    static sortKeys<T extends Partial<Record<string, unknown>> | unknown[]>(object: T, { deep, compare }?: {
-        deep?: boolean;
-        compare?: (x: string, y: string) => number;
-    }): T;
+    static sortKeys<T extends Partial<Record<string, unknown>> | unknown[]>(object: T, options?: ISortKeysOptions): T;
     static sortMapKeys<K, V>(map: Map<K, V>, keyComparer?: (x: K, y: K) => number): void;
     static sortSet<T>(set: Set<T>, comparer?: (x: T, y: T) => number): void;
     static sortSetBy<T>(set: Set<T>, 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
@@ -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<Record<string, unknown>> | unknown[],
+  Partial<Record<string, unknown>> | 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<T extends Partial<Record<string, unknown>> | 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<Record<string, unknown>> | unknown[],
-      Partial<Record<string, unknown>> | 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<Record<string, unknown>> | 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<Record<string, unknown>> | 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<Record<string, unknown>>): Partial<Record<string, unknown>> {
-      const resultFromCache: undefined | Partial<Record<string, unknown>> | unknown[] = cache.get(obj);
-      if (resultFromCache !== undefined) {
-        return resultFromCache as Partial<Record<string, unknown>>;
-      }
-      const result: Partial<Record<string, unknown>> = {};
-      const keys: string[] = Object.keys(obj).sort(compare);
+  return result;
+}
+function innerSortKeys(
+  obj: Partial<Record<string, unknown>>,
+  context: ISortKeysContext
+): Partial<Record<string, unknown>> {
+  const resultFromCache: undefined | Partial<Record<string, unknown>> | unknown[] = context.cache.get(obj);
+  if (resultFromCache !== undefined) {
+    return resultFromCache as Partial<Record<string, unknown>>;
+  }
+  const result: Partial<Record<string, unknown>> = {};
+  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
@@ -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
@@ -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']);