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

Make applyPatches to accept readonly Patch[] #1094

Merged
merged 3 commits into from
Apr 27, 2024

Conversation

Andarist
Copy link
Contributor

My main goal is for applyPatches to accept readonly Patch[] as there is no reason why it shouldn't accept it. If you think that other changes here are too risky (like returning readonly Patch[] from produceWithPatches) then I can revert those bits of this change.

@mweststrate
Copy link
Collaborator

Yeah that looks great! I'm not 100% sure about the returned patches indeed, as people might be postprocessing those in a mutable manner. So probably safer to not change right away. (Feel free to set up a separate PR to park for the next major)

@mweststrate
Copy link
Collaborator

Looks like some of the tests fail, but overall agree with the idea :)

@Andarist Andarist changed the title Make Patch[] arrays readonly on the outer bounds Make applyPatches to accept readonly Patch[] Mar 9, 2024
@Andarist
Copy link
Contributor Author

Andarist commented Mar 9, 2024

@mweststrate I adjusted the PR as requested. There are still some test failures here but they are unrelated to the change - the same ones can be seen main

@mweststrate
Copy link
Collaborator

I didn't use GitHub for too long and can't find the rebase button anymore, but master should be fixed now.

@Andarist
Copy link
Contributor Author

Just rebased this on top of main - it should be ready to go

@mweststrate
Copy link
Collaborator

Merging, thanks for the PR and sorry for the delay!

@mweststrate mweststrate merged commit 4da2e0d into immerjs:main Apr 27, 2024
Copy link
Contributor

🎉 This PR is included in version 10.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

3 participants