feat(utils): add `merge` and `isPlainObject`

cheton commented 1 week ago

PR Type

enhancement, tests

Description

Introduced merge function in shared.js to merge objects with deep clone capability.
Added isPlainObject function in assertion.js to check if a value is a plain object.
Enhanced test coverage by adding tests for merge and isPlainObject.
Refined existing tests for isEmptyArray, isEmptyObject, ariaAttr, and dataAttr.
Updated documentation to reflect new utility functions.

Changes walkthrough 📝

Relevant files

Enhancement

assertion.js Add `isPlainObject` function to assertion utilities packages/utils/src/assertion.js Added `isPlainObject` function to check if a value is a plain object.	+10/-0
shared.js Add `merge` function with deep clone capability packages/utils/src/shared.js Added `merge` function to merge objects. Implemented deep clone functionality for objects.	+37/-0

Tests

assertion.test.js Add tests for `isPlainObject` and refine existing tests packages/utils/src/__tests__/assertion.test.js Added tests for `isPlainObject`. Removed redundant test cases for `isEmptyArray` and `isEmptyObject`.	+50/-15
shared.test.js Add tests for `merge` function and refine attribute tests packages/utils/src/__tests__/shared.test.js Added tests for `merge` function. Separated tests for `ariaAttr` and `dataAttr`.	+126/-10
index.test.js `Update export tests to include new utilities` packages/utils/__tests__/index.test.js - Updated expected exports to include `isPlainObject` and `merge`.	+2/-0

Documentation

tonic-ui-945-utils.md

Document new utility functions in changeset

.changeset/tonic-ui-945-utils.md - Documented the addition of `merge` and `isPlainObject` functions.

+5/-0

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

codesandbox[bot] commented 1 week ago

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders
Open Preview

changeset-bot[bot] commented 1 week ago

🦋 Changeset detected

Latest commit: 90c3790beb680dde97b03855e7724e1cd5f6f986

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

| Name | Type | | --------------- | ----- | | @tonic-ui/utils | Minor |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codiumai-pr-agent-free[bot] commented 1 week ago

PR Reviewer Guide 🔍

(Review updated until commit https://github.com/trendmicro-frontend/tonic-ui/commit/0642b3dce56de7d2b154d98b212622b591f89d23)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 Security concerns

Prototype Pollution:
While the code includes tests for prototype pollution prevention, the merge function implementation could still be vulnerable to prototype pollution attacks through nested object properties that bypass the isPlainObject check. Additional validation may be needed for nested object properties.

⚡ Recommended focus areas for review

Possible Bug
The merge function may not properly handle circular references in objects that are not caught by WeakMap tracking Performance Issue
The isPlainObject implementation checks multiple Symbol properties which could be optimized by checking prototype chain first Code Smell
The _deepClone helper function is not exported but contains complex logic that should be tested independently

codesandbox-ci[bot] commented 1 week ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

codecov[bot] commented 1 week ago

Codecov Report

Attention: Patch coverage is 91.89189% with 3 lines in your changes missing coverage. Please review.

Project coverage is 78.41%. Comparing base (0b8452b) to head (90c3790). Report is 1 commits behind head on v2.

Files with missing lines	Patch %	Lines
packages/utils/src/shared.js	90.62%	3 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## v2 #946 +/- ## ========================================== + Coverage 78.35% 78.41% +0.06% ========================================== Files 405 405 Lines 6640 6677 +37 ========================================== + Hits 5203 5236 +33 - Misses 1437 1441 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

trendmicro-frontend-bot commented 1 week ago

Tonic UI Demo

On 2024-11-23 07:02:58 +0000, PR #946 (90c3790beb680dde97b03855e7724e1cd5f6f986) was successfully deployed. You can view it at the following link: https://trendmicro-frontend.github.io/tonic-ui-demo/react/pr-946/

cheton commented 1 week ago

/improve

cheton commented 1 week ago

/review

codiumai-pr-agent-free[bot] commented 1 week ago

Persistent review updated to latest commit https://github.com/trendmicro-frontend/tonic-ui/commit/65e7a61799c1a1a5c62d528cd2c6585855f91741

cheton commented 1 week ago

/describe

codiumai-pr-agent-free[bot] commented 1 week ago

PR Description updated to latest commit (https://github.com/trendmicro-frontend/tonic-ui/commit/65e7a61799c1a1a5c62d528cd2c6585855f91741)

[ ] Copy walkthrough table to "Files Changed" Tab

codiumai-pr-agent-free[bot] commented 1 week ago

PR Code Suggestions ✨

Latest suggestions up to 2c81177

Category	Suggestion	Score
Possible issue	✅ Add protection against circular references in deep cloning to prevent stack overflow errors ___ Suggestion Impact: The commit implemented a check for circular references in the _deepClone function using a WeakMap to prevent infinite recursion, as suggested. code diff: ```diff -const _deepClone = (source) => { +const _deepClone = (source, seen = new WeakMap()) => { + // Use a `WeakMap` to track objects and detect circular references. + // If the object has been cloned before, return the cached cloned version. + if (seen.has(source)) { + return seen.get(source); + } + if (Array.isArray(source)) { - return source.map((item) => _deepClone(item)); + const clonedArray = []; + seen.set(source, clonedArray); + for (let i = 0; i < source.length; ++i) { + clonedArray[i] = _deepClone(source[i], seen); + } + return clonedArray; } if (isPlainObject(source)) { - const output = {}; - Object.keys(source).forEach((key) => { - output[key] = _deepClone(source[key]); - }); - return output; + const clonedObject = {}; + seen.set(source, clonedObject); + for (const [key, value] of Object.entries(source)) { + clonedObject[key] = _deepClone(value, seen); + } + return clonedObject; } ``` ___ Consider adding a check for circular references in the `_deepClone` function to prevent infinite recursion when cloning objects with circular dependencies. [packages/utils/src/shared.js [18-33]](https://github.com/trendmicro-frontend/tonic-ui/pull/946/files#diff-aeb582188d4c096beb54627221b5a143210c728a566d8328830e6d31e1fee116R18-R33) ```diff -const _deepClone = (source) => { - if (Array.isArray(source)) { - return source.map((item) => _deepClone(item)); - } - - if (isPlainObject(source)) { - const output = {}; - Object.keys(source).forEach((key) => { - output[key] = _deepClone(source[key]); - }); +const _deepClone = (source, seen = new WeakMap()) => { + if (typeof source === 'object' && source !== null) { + if (seen.has(source)) { + return seen.get(source); + } + const output = Array.isArray(source) ? [] : {}; + seen.set(source, output); + + if (Array.isArray(source)) { + output.push(...source.map(item => _deepClone(item, seen))); + } else if (isPlainObject(source)) { + Object.keys(source).forEach((key) => { + output[key] = _deepClone(source[key], seen); + }); + } return output; } - return source; }; ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 9 Why: The suggestion addresses a critical security issue by preventing infinite recursion in deep cloning operations when circular references are present, which could cause stack overflow crashes.	9
Security	Add depth limit to prevent stack overflow from extremely deep recursive merges ___ Add a size limit check in the merge function to prevent potential stack overflow from extremely deep objects. [packages/utils/src/shared.js [60-65]](https://github.com/trendmicro-frontend/tonic-ui/pull/946/files#diff-aeb582188d4c096beb54627221b5a143210c728a566d8328830e6d31e1fee116R60-R65) ```diff -export const merge = (target, source, options = { clone: true }) => { +export const merge = (target, source, options = { clone: true }, depth = 0) => { + if (depth > 1000) { + throw new Error('Maximum merge depth exceeded'); + } // Merge arrays if (Array.isArray(target) && Array.isArray(source)) { const output = options.clone ? [...target] : target; source.forEach((item, index) => { ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 8 Why: This suggestion adds an important safety check to prevent potential DoS attacks through maliciously crafted deep objects that could cause stack overflow.	8
Enhancement	✅ Improve code readability and performance by using Object.entries() for object iteration ___ Suggestion Impact: The suggestion to use Object.entries() instead of Object.keys().forEach() was implemented in the commit, improving code readability and performance. code diff: ```diff + for (const [key, value] of Object.entries(source)) { + if (isPlainObject(value) && Object.prototype.hasOwnProperty.call(output, key) && isPlainObject(output[key])) { + output[key] = merge(output[key], value, options); } else { - output[key] = options.clone ? _deepClone(source[key]) : source[key]; + output[key] = options.clone ? _deepClone(value) : value; } - }); + } ``` ___ Consider using Object.entries() instead of Object.keys().forEach() for better readability and to avoid accessing object properties inside the loop. [packages/utils/src/shared.js [77-83]](https://github.com/trendmicro-frontend/tonic-ui/pull/946/files#diff-aeb582188d4c096beb54627221b5a143210c728a566d8328830e6d31e1fee116R77-R83) ```diff -Object.keys(source).forEach((key) => { - if (isPlainObject(source[key]) && isPlainObject(output[key]) && Object.prototype.hasOwnProperty.call(output, key)) { - output[key] = merge(output[key], source[key], options); +for (const [key, value] of Object.entries(source)) { + if (isPlainObject(value) && isPlainObject(output[key]) && Object.prototype.hasOwnProperty.call(output, key)) { + output[key] = merge(output[key], value, options); } else { - output[key] = options.clone ? _deepClone(source[key]) : source[key]; + output[key] = options.clone ? _deepClone(value) : value; } -}); +} ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 4 Why: While the suggestion improves code readability and slightly optimizes property access, the impact is relatively minor as the current implementation is already functional and clear.	4

Previous suggestions

✅ Suggestions up to commit 65e7a61

Category	Suggestion	Score
Enhancement	✅ Add special handling for array types in object merging function ___ Suggestion Impact: The commit added logic to handle merging of arrays in the merge function, which aligns with the suggestion to handle arrays properly. code diff: ```diff + // Merge arrays + if (Array.isArray(target) && Array.isArray(source)) { + const output = options.clone ? [...target] : target; + source.forEach((item, index) => { + if (isPlainObject(item) && isPlainObject(output[index])) { + output[index] = merge(output[index], item, options); } else { - output[key] = source[key]; + output[index] = options.clone ? _deepClone(item) : item; } }); + return output; ``` ___ The `merge` function should handle arrays properly since they are also objects but require different merging logic. [packages/utils/src/shared.js [60-61]](https://github.com/trendmicro-frontend/tonic-ui/pull/946/files#diff-aeb582188d4c096beb54627221b5a143210c728a566d8328830e6d31e1fee116R60-R61) ```diff +if (Array.isArray(source)) { + return options.clone ? [...source] : source; +} if (isPlainObject(target) && isPlainObject(source)) { Object.keys(source).forEach((key) => { ``` Suggestion importance[1-10]: 8 Why: The suggestion addresses an important edge case in the merge function by properly handling arrays, which could prevent unexpected behavior when merging array-containing objects.	8
Possible bug	Add defensive programming by validating input parameters before processing ___ Consider adding a null/undefined check for the source parameter to prevent potential runtime errors when merging objects. [packages/utils/src/shared.js [57-58]](https://github.com/trendmicro-frontend/tonic-ui/pull/946/files#diff-aeb582188d4c096beb54627221b5a143210c728a566d8328830e6d31e1fee116R57-R58) ```diff export const merge = (target, source, options = { clone: true }) => { + if (!source) return target; const output = options.clone ? { ...target } : target; ``` Suggestion importance[1-10]: 7 Why: Adding a null/undefined check for the source parameter is a good defensive programming practice that prevents potential runtime errors and improves code robustness.	7
Best practice	Improve type checking accuracy for plain objects ___ Add a check for primitive values wrapped in Object to ensure more accurate plain object detection. [packages/utils/src/assertion.js [34-37]](https://github.com/trendmicro-frontend/tonic-ui/pull/946/files#diff-9bb98fab345c19425bb6bf765ba09c993de88b5f9fd1e21b6a8023d23bebcef3R34-R37) ```diff export const isPlainObject = (value) => { - if (typeof value !== 'object' \|\| value === null) { + if (typeof value !== 'object' \|\| value === null \|\| Object(value) !== value) { return false; } ``` Suggestion importance[1-10]: 6 Why: The additional check for primitive values wrapped in Object would make the isPlainObject function more robust, though the current implementation already handles most common cases correctly.	6

Suggestions up to commit 5136f4e

Category	Suggestion	Score
Possible bug	Add input validation to prevent errors from invalid arguments ___ Add input validation to ensure target and source are objects before proceeding with the merge. This prevents potential errors when invalid inputs are provided. [packages/utils/src/shared.js [57-58]](https://github.com/trendmicro-frontend/tonic-ui/pull/946/files#diff-aeb582188d4c096beb54627221b5a143210c728a566d8328830e6d31e1fee116R57-R58) ```diff export const deepmerge = (target, source, options = { clone: true }) => { + if (!target \|\| typeof target !== 'object') return source; + if (!source \|\| typeof source !== 'object') return target; const output = options.clone ? { ...target } : target; ``` Suggestion importance[1-10]: 8 Why: The suggestion adds crucial input validation to prevent potential runtime errors and unexpected behavior when invalid arguments are passed to the deepmerge function.	8
Best practice	Use modern Object.hasOwn() method for property ownership checks ___ Consider using Object.hasOwn() instead of Object.prototype.hasOwnProperty.call() for a more modern and safer property ownership check. [packages/utils/src/shared.js [64]](https://github.com/trendmicro-frontend/tonic-ui/pull/946/files#diff-aeb582188d4c096beb54627221b5a143210c728a566d8328830e6d31e1fee116R64-R64) ```diff -Object.prototype.hasOwnProperty.call(target, key) +Object.hasOwn(target, key) ``` Suggestion importance[1-10]: 5 Why: Using Object.hasOwn() is a cleaner and more modern approach than Object.prototype.hasOwnProperty.call(), though both are functionally correct. The improvement is mainly for code maintainability.	5
Performance	Cache repeated function call results to improve performance ___ Cache the result of Object.getPrototypeOf(value) to avoid multiple calls to the same function with the same argument. [packages/utils/src/assertion.js [39-40]](https://github.com/trendmicro-frontend/tonic-ui/pull/946/files#diff-9bb98fab345c19425bb6bf765ba09c993de88b5f9fd1e21b6a8023d23bebcef3R39-R40) ```diff const prototype = Object.getPrototypeOf(value); -return (prototype === null \|\| prototype === Object.prototype \|\| Object.getPrototypeOf(prototype) === null) && !(Symbol.toStringTag in value) && !(Symbol.iterator in value); +const prototypeOfPrototype = prototype && Object.getPrototypeOf(prototype); +return (prototype === null \|\| prototype === Object.prototype \|\| prototypeOfPrototype === null) && !(Symbol.toStringTag in value) && !(Symbol.iterator in value); ``` Suggestion importance[1-10]: 4 Why: The suggestion optimizes performance by avoiding a repeated Object.getPrototypeOf() call, though the performance impact would be minimal in most use cases.	4

codiumai-pr-agent-free[bot] commented 1 week ago

Persistent review updated to latest commit https://github.com/trendmicro-frontend/tonic-ui/commit/2c8117714c2698228296539f6177eb2c0598c3bc

cheton commented 1 week ago

/review

cheton commented 1 week ago

/improve

codiumai-pr-agent-free[bot] commented 1 week ago

PR Code Suggestions ✨

No code suggestions found for the PR.

codiumai-pr-agent-free[bot] commented 1 week ago

Persistent review updated to latest commit https://github.com/trendmicro-frontend/tonic-ui/commit/0642b3dce56de7d2b154d98b212622b591f89d23

cheton commented 3 days ago

/ask enhance test coverae for the merge function with arrays

codiumai-pr-agent-free[bot] commented 3 days ago

Ask❓

enhance test coverae for the merge function with arrays

Answer:

To enhance test coverage for the merge function specifically with arrays, you can add test cases that cover various scenarios involving array merging. Here are some suggestions for additional test cases:

Merging Arrays with Primitive Values:

Test merging two arrays containing primitive values (e.g., numbers, strings) to ensure they are combined correctly.

it('should merge arrays with primitive values', () => {
 const target = [1, 2, 3];
 const source = [4, 5, 6];
 const result = merge(target, source);
 expect(result).toEqual([4, 5, 6]);
});

Merging Arrays with Objects:

Test merging arrays where elements are objects to ensure deep merging of objects within arrays.

it('should merge arrays with objects', () => {
 const target = [{ a: 1 }, { b: 2 }];
 const source = [{ a: 3 }, { c: 4 }];
 const result = merge(target, source);
 expect(result).toEqual([{ a: 3 }, { c: 4 }]);
});

Handling Arrays of Different Lengths:

Test merging arrays of different lengths to verify that the longer array's elements are preserved.

it('should handle arrays of different lengths', () => {
 const target = [1, 2];
 const source = [3, 4, 5];
 const result = merge(target, source);
 expect(result).toEqual([3, 4, 5]);
});

Merging Nested Arrays:

Test merging arrays that contain nested arrays to ensure proper handling of nested structures.

it('should merge nested arrays', () => {
 const target = [[1, 2], [3, 4]];
 const source = [[5, 6], [7, 8]];
 const result = merge(target, source);
 expect(result).toEqual([[5, 6], [7, 8]]);
});

Ensuring Cloning Behavior:

Test the cloning behavior when merging arrays to ensure that changes to the result do not affect the original arrays.

it('should clone arrays when merging', () => {
 const target = [1, 2, 3];
 const source = [4, 5, 6];
 const result = merge(target, source, { clone: true });
 result[0] = 10;
 expect(target).toEqual([1, 2, 3]);
 expect(source).toEqual([4, 5, 6]);
});

By adding these test cases, you can ensure that the merge function behaves correctly when dealing with arrays in various scenarios.

trendmicro-frontend / tonic-ui