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

Fix Tree copy by doing a shallow copy of the value #5081

Merged
merged 2 commits into from
Oct 12, 2023

Conversation

hugo-vrijswijk
Copy link
Contributor

Fixes #5080

This does a shallow clone of the value, instead of a deep clone. This way data is not modified. The copy is only 1 level deep. In testing, this worked fine. But someone with more knowledge of the code should review and decide if a deeper copy (excluding data) is needed.

@vercel
Copy link

vercel bot commented Oct 11, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
primereact ⬜️ Ignored (Inspect) Visit Preview Oct 12, 2023 2:55pm

@melloware melloware added the Status: Pending Review Issue or pull request is being reviewed by Core Team label Oct 11, 2023
@melloware
Copy link
Member

melloware commented Oct 11, 2023

Hmm I am not sure about doing just a shallow copy. I see your issue with the circular reference but aren't you better off modifying your model object being used in the treenode than to make this assumption of 1 level deep?

@hugo-vrijswijk
Copy link
Contributor Author

I could write a deep clone that leaves the data property alone. That way it's still recursive but would solve my issue and for anyone using non-serializable data. What do you think?
I unfortunately can't easily change my code to only support serializable data.

@melloware
Copy link
Member

OK yeah i would do a deep copy without the data attribute as a start.

@melloware
Copy link
Member

something like this..

function deepCopyWithoutData(obj) {
  if (obj === null || typeof obj !== 'object') {
    return obj;
  }

  if (Array.isArray(obj)) {
    return obj.map(item => deepCopyWithoutData(item));
  }

  const result = {};
  for (let key in obj) {
    if (key !== 'data') {
      result[key] = deepCopyWithoutData(obj[key]);
    }
  }

  return result;
}

@hugo-vrijswijk
Copy link
Contributor Author

Thanks, I made a sort of similar implementation that handles some edge-cases. JavaScript is messy 😅

@hugo-vrijswijk
Copy link
Contributor Author

@melloware Is this PR good to merge like this? I've tested it both in the docs and my own project and it works on both

@melloware melloware added Type: Enhancement Issue contains an enhancement related to a specific component. Additional functionality has been add and removed Status: Pending Review Issue or pull request is being reviewed by Core Team labels Oct 12, 2023
@melloware melloware merged commit 8f3e493 into primefaces:master Oct 12, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Issue contains an enhancement related to a specific component. Additional functionality has been add
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tree: onDrop should not attempt a copy of node values
3 participants