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

[minor] Preserve insertion order of Sets, fixes #819 #976

Merged
merged 2 commits into from Jan 15, 2023

Conversation

jdip
Copy link
Contributor

@jdip jdip commented Sep 9, 2022

As pointed out in #819 insertion order is not always preserved when adding to a Set, though according to MDN it should be.

We were already cloning and iterating over the cloned Set anyway in finalize.

If we take it a step further to clear the original Set after cloning and let finalizeProperty know it needs to re-add non-draftable children back to the target then we end up rebuilding the Set in it's original order.

We then no longer need to delete anything from the Set in set

We are already cloning and iterating over the Set. If we go a step further and clear it before iteration, and re-add all children during iteration, we can preserve insertion order.
@netlify
Copy link

netlify bot commented Sep 9, 2022

Deploy Preview for quizzical-lovelace-dcbd6a canceled.

Name Link
🔨 Latest commit ab864a6
🔍 Latest deploy log https://app.netlify.com/sites/quizzical-lovelace-dcbd6a/deploys/631a8c5ad5760200091410bc

@mweststrate
Copy link
Collaborator

Sorry for the late follow up! Changes look legit, will include in the next minor.

@mweststrate mweststrate changed the title Proposed fix for #819 [minor] Preserve insertion order of Sets, fixes #819 Jan 15, 2023
@mweststrate mweststrate merged commit b3eeb69 into immerjs:master Jan 15, 2023
@github-actions
Copy link
Contributor

🎉 This PR is included in version 9.0.18 🎉

The release is available on:

Your semantic-release bot 📦🚀

@jdip
Copy link
Contributor Author

jdip commented Jan 16, 2023

Sorry for the late follow up! Changes look legit, will include in the next minor.

No worries. Thanks for taking the time to review/include it!

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

Successfully merging this pull request may close these issues.

None yet

2 participants