Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(utils): add deepmerge and isPlainObject #946

Open
wants to merge 2 commits into
base: v2
Choose a base branch
from
Open

Conversation

cheton
Copy link
Member

@cheton cheton commented Nov 12, 2024

PR Type

Enhancement, Tests


Description

  • Added isPlainObject function to determine if a value is a plain object.
  • Implemented deepmerge function to merge objects with options for cloning.
  • Introduced _deepClone helper function for deep cloning objects.
  • Added comprehensive tests for isPlainObject and deepmerge functions.
  • Updated test files to include new utility functions in expected exports.

Changes walkthrough 📝

Relevant files
Enhancement
assertion.js
Add `isPlainObject` function for object validation             

packages/utils/src/assertion.js

  • Added isPlainObject function to check if a value is a plain object.
  • +10/-0   
    shared.js
    Implement `deepmerge` and `_deepClone` functions for object handling

    packages/utils/src/shared.js

  • Added deepmerge function to merge objects.
  • Implemented _deepClone helper function for deep cloning 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 `deepmerge` and separate attribute tests     

    packages/utils/src/tests/shared.test.js

  • Added tests for deepmerge function.
  • Separated tests for ariaAttr and dataAttr.
  • +126/-10
    index.test.js
    Update expected exports to include new utilities                 

    packages/utils/tests/index.test.js

    • Added isPlainObject and deepmerge to expected exports.
    +2/-0     
    index.test.js
    Update expected exports to include new utilities                 

    packages/utils/src/tests/index.test.js

    • Added isPlainObject and deepmerge to expected exports.
    +2/-0     

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

    Copy link

    codesandbox bot commented Nov 12, 2024

    Review or Edit in CodeSandbox

    Open the branch in Web EditorVS CodeInsiders

    Open Preview

    Copy link

    changeset-bot bot commented Nov 12, 2024

    🦋 Changeset detected

    Latest commit: cd713f4

    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

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 Security concerns

    Prototype Pollution:
    The deepmerge function is vulnerable to prototype pollution attacks as demonstrated in the test cases. The function allows modification of object prototypes through special properties like proto, constructor.prototype, which could lead to security issues if untrusted data is merged. Consider implementing safeguards against prototype pollution.

    ⚡ Recommended focus areas for review

    Prototype Pollution
    The deepmerge function allows prototype pollution as shown in the tests. Consider adding protection against prototype pollution by checking for special property names like proto, constructor, and prototype.

    Performance Issue
    The deepmerge function creates a new object spread on every recursive call, which could be inefficient for deeply nested objects. Consider optimizing the clone operation.

    Edge Case
    The isPlainObject implementation might not handle all edge cases with Symbol properties. Consider adding more test cases for objects with custom Symbol properties.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    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]

     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;
    • Apply this suggestion
    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]

    -Object.prototype.hasOwnProperty.call(target, key)
    +Object.hasOwn(target, key)
    • Apply this suggestion
    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]

     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);
    • Apply this suggestion
    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

    💡 Need additional feedback ? start a PR chat

    Copy link

    codesandbox-ci bot commented Nov 12, 2024

    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.

    Copy link

    codecov bot commented Nov 12, 2024

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 78.41%. Comparing base (0b8452b) to head (cd713f4).

    Additional details and impacted files
    @@            Coverage Diff             @@
    ##               v2     #946      +/-   ##
    ==========================================
    + Coverage   78.35%   78.41%   +0.05%     
    ==========================================
      Files         405      405              
      Lines        6640     6662      +22     
    ==========================================
    + Hits         5203     5224      +21     
    - Misses       1437     1438       +1     

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    @trendmicro-frontend-bot
    Copy link
    Contributor

    trendmicro-frontend-bot commented Nov 12, 2024

    Tonic UI Demo

    On 2024-11-12 16:08:55 +0000, PR #946 (cd713f4) was successfully deployed. You can view it at the following link:
    https://trendmicro-frontend.github.io/tonic-ui-demo/react/pr-946/

    Copy link
    Contributor

    CI Failure Feedback 🧐

    Action: deploy

    Failed stage: Deploy to gh-pages [❌]

    Failure summary:

    The action failed because the push to the remote repository was rejected.

  • The rejection occurred because the remote branch gh-pages contains work that is not present in the
    local branch.
  • This is typically caused by another repository pushing to the same branch, causing a conflict.
  • To resolve this, a git pull should be performed to integrate the remote changes before attempting to
    push again.

  • Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    291:  ./_next/static/chunks/webpack-f06def3a6fdced5a.js
    292:  ./_next/static/chunks/4791-5b920d11f6acba4f.js
    293:  ./_next/static/chunks/4575-fcf235c18d03e610.js
    294:  ./_next/static/chunks/1630-2e46e2811276207a.js
    295:  ./_next/static/chunks/7641-cd82f85a940255e7.js
    296:  ./_next/static/chunks/6314-ddc4e0b5b6964ea0.js
    297:  ./_next/static/chunks/pages/
    298:  ./_next/static/chunks/pages/styled-system-1f2b809fc7b7c127.js
    299:  ./_next/static/chunks/pages/_error-655c54429e8e39bb.js
    ...
    
    576:  ./components/color-mode/light-mode.html
    577:  ./components/menu.html
    578:  ./components/stack.html
    579:  ./images/
    580:  ./images/Trend-Micro-Logo.svg
    581:  ./images/patterns/
    582:  ./images/patterns/notification/
    583:  ./images/patterns/notification/icon-notification-info.svg
    584:  ./images/patterns/notification/icon-notification-error.svg
    ...
    
    694:  create mode 100644 react/pr-946/_next/static/chunks/9050.c73e8e7a188a83f8.js
    695:  create mode 100644 react/pr-946/_next/static/chunks/9293-f15268b2ecc2147f.js
    696:  create mode 100644 react/pr-946/_next/static/chunks/978-906158bad5e4cbf7.js
    697:  create mode 100644 react/pr-946/_next/static/chunks/9860-78f03667ad77447d.js
    698:  create mode 100644 react/pr-946/_next/static/chunks/framework-a32fdada02556615.js
    699:  create mode 100644 react/pr-946/_next/static/chunks/main-3f4c4208008ae0f1.js
    700:  create mode 100644 react/pr-946/_next/static/chunks/pages/404-e246ffc50d3c4e0b.js
    701:  create mode 100644 react/pr-946/_next/static/chunks/pages/_app-7426264c39cbac4b.js
    702:  create mode 100644 react/pr-946/_next/static/chunks/pages/_error-655c54429e8e39bb.js
    ...
    
    952:  create mode 100644 react/pr-946/hooks/useOnce.html
    953:  create mode 100644 react/pr-946/hooks/useOnceWhen.html
    954:  create mode 100644 react/pr-946/hooks/useOutsideClick.html
    955:  create mode 100644 react/pr-946/hooks/usePrevious.html
    956:  create mode 100644 react/pr-946/hooks/useToggle.html
    957:  create mode 100644 react/pr-946/icons.html
    958:  create mode 100644 react/pr-946/icons/svg-icon.html
    959:  create mode 100644 react/pr-946/images/Trend-Micro-Logo.svg
    960:  create mode 100644 react/pr-946/images/patterns/notification/icon-notification-error.svg
    ...
    
    993:  create mode 100644 react/pr-946/theme/radii.html
    994:  create mode 100644 react/pr-946/theme/shadows.html
    995:  create mode 100644 react/pr-946/theme/sizes.html
    996:  create mode 100644 react/pr-946/theme/space.html
    997:  create mode 100644 react/pr-946/theme/z-indices.html
    998:  create mode 100644 react/pr-946/tonic-favicon-dark.ico
    999:  To github.com:trendmicro-frontend/tonic-ui-demo.git
    1000:  ! [rejected]          gh-pages -> gh-pages (fetch first)
    1001:  error: failed to push some refs to 'github.com:trendmicro-frontend/tonic-ui-demo.git'
    1002:  hint: Updates were rejected because the remote contains work that you do not
    1003:  hint: have locally. This is usually caused by another repository pushing to
    1004:  hint: the same ref. If you want to integrate the remote changes, use
    1005:  hint: 'git pull' before pushing again.
    1006:  hint: See the 'Note about fast-forwards' in 'git push --help' for details.
    1007:  ##[error]Process completed with exit code 1.
    

    ✨ CI feedback usage guide:

    The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
    The tool analyzes the failed checks and provides several feedbacks:

    • Failed stage
    • Failed test name
    • Failure summary
    • Relevant error logs

    In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

    /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"
    

    where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

    Configuration options

    • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
    • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
    • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
    • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
    • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

    See more information about the checks tool in the docs.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants