From ed9dc1d695162ec9a4e2beaf0033963588f8e1b4 Mon Sep 17 00:00:00 2001 From: Kenrick Date: Tue, 10 Sep 2024 11:17:14 +0800 Subject: [PATCH] fix: code review --- ...ap-packageExtensions_2024-09-06-03-55.json | 2 +- ...ap-packageExtensions_2024-09-06-03-55.json | 2 +- libraries/node-core-library/src/Sort.ts | 26 ++++--- .../node-core-library/src/test/Sort.test.ts | 68 +++++++------------ .../installManager/WorkspaceInstallManager.ts | 16 +++-- 5 files changed, 52 insertions(+), 62 deletions(-) diff --git a/common/changes/@microsoft/rush/kenrick-fix-update-shrinkwrap-packageExtensions_2024-09-06-03-55.json b/common/changes/@microsoft/rush/kenrick-fix-update-shrinkwrap-packageExtensions_2024-09-06-03-55.json index deaa6a64bf8..562bec8e668 100644 --- a/common/changes/@microsoft/rush/kenrick-fix-update-shrinkwrap-packageExtensions_2024-09-06-03-55.json +++ b/common/changes/@microsoft/rush/kenrick-fix-update-shrinkwrap-packageExtensions_2024-09-06-03-55.json @@ -2,7 +2,7 @@ "changes": [ { "packageName": "@microsoft/rush", - "comment": "Always update shrinkwrap when globalPackageExtensions has been changed", + "comment": "Always update shrinkwrap when `globalPackageExtensions` in `common/config/rush/pnpm-config.json` has been changed.", "type": "none" } ], diff --git a/common/changes/@rushstack/node-core-library/kenrick-fix-update-shrinkwrap-packageExtensions_2024-09-06-03-55.json b/common/changes/@rushstack/node-core-library/kenrick-fix-update-shrinkwrap-packageExtensions_2024-09-06-03-55.json index 32953c4e722..c63a92f4177 100644 --- a/common/changes/@rushstack/node-core-library/kenrick-fix-update-shrinkwrap-packageExtensions_2024-09-06-03-55.json +++ b/common/changes/@rushstack/node-core-library/kenrick-fix-update-shrinkwrap-packageExtensions_2024-09-06-03-55.json @@ -2,7 +2,7 @@ "changes": [ { "packageName": "@rushstack/node-core-library", - "comment": "Add Sort.sortKeys for sorting keys in an object", + "comment": "Add a `Sort.sortKeys` function for sorting keys in an object", "type": "minor" } ], diff --git a/libraries/node-core-library/src/Sort.ts b/libraries/node-core-library/src/Sort.ts index fd02c9ec899..1d340836241 100644 --- a/libraries/node-core-library/src/Sort.ts +++ b/libraries/node-core-library/src/Sort.ts @@ -12,6 +12,7 @@ export interface ISortKeysOptions { * @defaultValue false */ deep?: boolean; + /** * Custom compare function when sorting the keys * @@ -297,6 +298,7 @@ export class Sort { : (innerSortKeys(object, context) as T); } } + function isPlainObject(obj: unknown): obj is object { return obj !== null && typeof obj === 'object'; } @@ -309,22 +311,24 @@ function innerSortArray(arr: unknown[], context: ISortKeysContext): 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; - }) - ); + for (const entry of arr) { + if (Array.isArray(entry)) { + result.push(innerSortArray(entry, context)); + } else if (isPlainObject(entry)) { + result.push(innerSortKeys(entry, context)); + } else { + result.push(entry); + } + } } else { - result.push(...arr); + for (const entry of arr) { + result.push(entry); + } } return result; } + function innerSortKeys( obj: Partial>, context: ISortKeysContext diff --git a/libraries/node-core-library/src/test/Sort.test.ts b/libraries/node-core-library/src/test/Sort.test.ts index 9b8be981a5c..c252b23b299 100644 --- a/libraries/node-core-library/src/test/Sort.test.ts +++ b/libraries/node-core-library/src/test/Sort.test.ts @@ -73,21 +73,14 @@ test('Sort.sortSet', () => { describe('Sort.sortKeys', () => { // Test cases from https://github.com/sindresorhus/sort-keys/blob/v4.2.0/test.js function deepEqualInOrder( - // eslint-disable-next-line @typescript-eslint/no-explicit-any - actual: Partial>, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - expected: Partial> + actual: Partial>, + expected: Partial> ): void { expect(actual).toEqual(expected); - const seen = new Set(); + const seen: Set = new Set(); - function assertSameKeysInOrder( - // eslint-disable-next-line @typescript-eslint/no-explicit-any - object1: Partial>, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - object2: Partial> - ): void { + function assertSameKeysInOrder(object1: unknown, object2: unknown): void { // This function assumes the objects given are already deep equal. if (seen.has(object1) && seen.has(object2)) { @@ -97,16 +90,24 @@ describe('Sort.sortKeys', () => { seen.add(object1); seen.add(object2); - if (Array.isArray(object1)) { + if (Array.isArray(object1) && Array.isArray(object2)) { for (const index of object1.keys()) { assertSameKeysInOrder(object1[index], object2[index]); } - } else if (typeof object1 === 'object') { + } else if ( + typeof object1 === 'object' && + typeof object2 === 'object' && + object1 != null && + object2 != null + ) { const keys1 = Object.keys(object1); const keys2 = Object.keys(object2); expect(keys1).toEqual(keys2); for (const index of keys1.keys()) { - assertSameKeysInOrder(object1[keys1[index]], object2[keys2[index]]); + assertSameKeysInOrder( + (object1 as Partial>)[keys1[index]], + (object2 as Partial>)[keys2[index]] + ); } } } @@ -135,13 +136,6 @@ describe('Sort.sortKeys', () => { c: { a: 0, b: 0, c: 0 } }); - expect(() => { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const object: Partial> = { a: 0 }; - object.circular = object; - Sort.sortKeys(object, { deep: true }); - }).not.toThrow(); - // eslint-disable-next-line @typescript-eslint/no-explicit-any const object: Partial> = { z: 0 }; object.circular = object; @@ -169,20 +163,13 @@ describe('Sort.sortKeys', () => { object3.a[0].a = object4.a[0]; object4.a[0].c = object3.a[0]; - expect(() => { - Sort.sortKeys(object1, { deep: true }); - Sort.sortKeys(object2, { deep: true }); - Sort.sortKeys(object3, { deep: true }); - Sort.sortKeys(object4, { deep: true }); - }).not.toThrow(); - - const sorted = Sort.sortKeys(object1, { deep: true }); - const deepSorted = Sort.sortKeys(object3, { deep: true }); + const sortedObject1 = Sort.sortKeys(object1, { deep: true }); + const sortedObject3 = Sort.sortKeys(object3, { deep: true }); - expect(sorted).toBe(sorted.a.c); - deepEqualInOrder(deepSorted.a[0], deepSorted.a[0].a.c); - expect(Object.keys(sorted)).toStrictEqual(['a', 'b']); - expect(Object.keys(deepSorted.a[0])).toStrictEqual(['a', 'b']); + expect(sortedObject1).toBe(sortedObject1.a.c); + deepEqualInOrder(sortedObject3.a[0], sortedObject3.a[0].a.c); + expect(Object.keys(sortedObject1)).toStrictEqual(['a', 'b']); + expect(Object.keys(sortedObject3.a[0])).toStrictEqual(['a', 'b']); deepEqualInOrder( Sort.sortKeys({ c: { c: 0, a: 0, b: 0 }, a: 0, b: 0, z: [9, 8, 7, 6, 5] }, { deep: true }), { a: 0, b: 0, c: { a: 0, b: 0, c: 0 }, z: [9, 8, 7, 6, 5] } @@ -199,10 +186,6 @@ describe('Sort.sortKeys', () => { object.a.push(object); object.a[1].push(object.a[1]); - expect(() => { - Sort.sortKeys(object, { deep: true }); - }).not.toThrow(); - const sorted = Sort.sortKeys(object, { deep: true }); // Cannot use .toBe() as Jest will encounter https://github.com/jestjs/jest/issues/10577 expect(sorted.a[2] === sorted).toBeTruthy(); @@ -213,8 +196,7 @@ describe('Sort.sortKeys', () => { }); test('top-level array', () => { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const array: Array = [ + const array: Array>> = [ { b: 0, a: 0 }, { c: 0, d: 0 } ]; @@ -232,8 +214,7 @@ describe('Sort.sortKeys', () => { }); test('keeps property descriptors intact', () => { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const descriptors: Partial> = { + const descriptors: PropertyDescriptorMap = { b: { value: 1, configurable: true, @@ -248,8 +229,7 @@ describe('Sort.sortKeys', () => { } }; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const object: Partial> = {}; + const object: Partial> = {}; Object.defineProperties(object, descriptors); const sorted = Sort.sortKeys(object); diff --git a/libraries/rush-lib/src/logic/installManager/WorkspaceInstallManager.ts b/libraries/rush-lib/src/logic/installManager/WorkspaceInstallManager.ts index f582da32357..117dd9bbfa9 100644 --- a/libraries/rush-lib/src/logic/installManager/WorkspaceInstallManager.ts +++ b/libraries/rush-lib/src/logic/installManager/WorkspaceInstallManager.ts @@ -405,16 +405,12 @@ export class WorkspaceInstallManager extends BaseInstallManager { private _getPackageExtensionChecksum( packageExtensions: Record | undefined ): string | undefined { + // https://github.com/pnpm/pnpm/blob/ba9409ffcef0c36dc1b167d770a023c87444822d/pkg-manager/core/src/install/index.ts#L331 const packageExtensionsChecksum: string | undefined = Object.keys(packageExtensions ?? {}).length === 0 ? undefined : createObjectChecksum(packageExtensions!); - function createObjectChecksum(obj: Record): string { - const s: string = JSON.stringify(Sort.sortKeys(obj, { deep: true })); - return createHash('md5').update(s).digest('hex'); - } - return packageExtensionsChecksum; } @@ -774,3 +770,13 @@ export class WorkspaceInstallManager extends BaseInstallManager { } } } + +/** + * Source: https://github.com/pnpm/pnpm/blob/ba9409ffcef0c36dc1b167d770a023c87444822d/pkg-manager/core/src/install/index.ts#L821-L824 + * @param obj + * @returns + */ +function createObjectChecksum(obj: Record): string { + const s: string = JSON.stringify(Sort.sortKeys(obj, { deep: true })); + return createHash('md5').update(s).digest('hex'); +}