Skip to content

Commit

Permalink
fix: code review
Browse files Browse the repository at this point in the history
  • Loading branch information
@kenrick95
kenrick95 committed Sep 10, 2024
1 parent 343bd55 commit ed9dc1d
Show file tree
Hide file tree
Showing 5 changed files with 52 additions and 62 deletions.
2 changes: 1 addition & 1 deletion ...ges/@microsoft/rush/kenrick-fix-update-shrinkwrap-packageExtensions_2024-09-06-03-55.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
],
Expand Down
2 changes: 1 addition & 1 deletion ...k/node-core-library/kenrick-fix-update-shrinkwrap-packageExtensions_2024-09-06-03-55.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
],
Expand Down
26 changes: 15 additions & 11 deletions libraries/node-core-library/src/Sort.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export interface ISortKeysOptions {
* @defaultValue false
*/
deep?: boolean;

/**
* Custom compare function when sorting the keys
*
Expand Down Expand Up @@ -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';
}
Expand All @@ -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<Record<string, unknown>>,
context: ISortKeysContext
Expand Down
68 changes: 24 additions & 44 deletions libraries/node-core-library/src/test/Sort.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Record<string, any>>,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
expected: Partial<Record<string, any>>
actual: Partial<Record<string, unknown>>,
expected: Partial<Record<string, unknown>>
): void {
expect(actual).toEqual(expected);

const seen = new Set();
const seen: Set<unknown> = new Set();

function assertSameKeysInOrder(
// eslint-disable-next-line @typescript-eslint/no-explicit-any
object1: Partial<Record<string, any>>,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
object2: Partial<Record<string, any>>
): 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)) {
Expand All @@ -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<Record<string, unknown>>)[keys1[index]],
(object2 as Partial<Record<string, unknown>>)[keys2[index]]
);
}
}
}
Expand Down Expand Up @@ -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<Record<string, any>> = { a: 0 };
object.circular = object;
Sort.sortKeys(object, { deep: true });
}).not.toThrow();

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const object: Partial<Record<string, any>> = { z: 0 };
object.circular = object;
Expand Down Expand Up @@ -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] }
Expand All @@ -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();
Expand All @@ -213,8 +196,7 @@ describe('Sort.sortKeys', () => {
});

test('top-level array', () => {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const array: Array<any> = [
const array: Array<Partial<Record<string, number>>> = [
{ b: 0, a: 0 },
{ c: 0, d: 0 }
];
Expand All @@ -232,8 +214,7 @@ describe('Sort.sortKeys', () => {
});

test('keeps property descriptors intact', () => {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const descriptors: Partial<Record<string, any>> = {
const descriptors: PropertyDescriptorMap = {
b: {
value: 1,
configurable: true,
Expand All @@ -248,8 +229,7 @@ describe('Sort.sortKeys', () => {
}
};

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const object: Partial<Record<string, any>> = {};
const object: Partial<Record<string, unknown>> = {};
Object.defineProperties(object, descriptors);

const sorted = Sort.sortKeys(object);
Expand Down
16 changes: 11 additions & 5 deletions libraries/rush-lib/src/logic/installManager/WorkspaceInstallManager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -405,16 +405,12 @@ export class WorkspaceInstallManager extends BaseInstallManager {
private _getPackageExtensionChecksum(
packageExtensions: Record<string, unknown> | 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, unknown>): string {
const s: string = JSON.stringify(Sort.sortKeys(obj, { deep: true }));
return createHash('md5').update(s).digest('hex');
}

return packageExtensionsChecksum;
}

Expand Down Expand Up @@ -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, unknown>): string {
const s: string = JSON.stringify(Sort.sortKeys(obj, { deep: true }));
return createHash('md5').update(s).digest('hex');
}

0 comments on commit ed9dc1d

Please sign in to comment.