search

charkour / zundo

🍜 undo/redo middleware for zustand. <700 bytes
https://codesandbox.io/s/zundo-2dom9
MIT License
597 stars 19 forks source link

handleSet refactor option 3 #149

Closed pstrassmann closed 7 months ago

pstrassmann commented 8 months ago

This PR is to explore the solution space around ideas around this issue, specifically based on this comment

TL;DR This PR and PR 141 look very similar now at first glance. To see the primary difference, note how curriedHandleSet takes different numbers of parameters in the two PRs.

Background: In PR 141, there are zero changes to the ZundoOptions API. However, in bringing the state change check in prior to calling curriedHandleSet, we have the minorish drawback of having to calculate currentState and deltaState BOTH 1) to calculate the check for calling curriedHandleSet and 2) within curriedHandleSet.

An idea was then -- instead of calculating those values twice, maybe we just calculate those things once and just pass them to curriedHandleSet. So that's what we do in this PR.

While we resolve the redundant calculation issue, the drawback though is that we cannot both let curriedHandleSet accept more parameters and not simultaneously have absolutely zero changes to the API. Like PR 142, options.handleSet now returns a function that, in addition to pastState as before, now technically also receives the values of currentState and deltaState. So like in PR 142, the minor change to the API is that users now also have access to these values and can use them in logic (although in this PR they no longer need to to resolve the throttling/untrackedValue issues themselves with additional logic, as that is now, like in PR 141, always resolved by the conditional check if equality or diff functions exist.

All that being said, I believe this change (this PR) is non breaking and all current compositions of handleSet will continue to work as expected (the same is true of the other PRs as well). All tests pass, and this was tested locally with previous compositions of handleSet.

pstrassmann commented 8 months ago

@charkour Hey! Sorry I will get to all of this this week! Thank you!

charkour commented 8 months ago

No rush. Happy new year!

pstrassmann commented 8 months ago

@charkour Yes this PR I think is the best of all the options. Happy to close out the other ones and merge this one in. Very happy we were able to land on a solution without any breaking changes to the API! Thanks again for all your help on this, and I'm looking forward to helping grow and improve zundo! It is really amazing how quickly zundo enabled our application at Classy to have performant undo/redo functionality across our features. Zustand is the best, and made even better with great extensions like the one you've built!

Happy to address any additional feedback on this PR as well. Thank you!

charkour commented 8 months ago

Thanks for the awesome work! I'm glad that zundo is very helpful when adding new features to your platform at work.

I plan to review this soon and would like to release a new version this weekend. Sorry for the delay on this!

pstrassmann commented 7 months ago

Thank you!! 🙏

pstrassmann commented 7 months ago

@charkour Ok sounds good! I'll find some time in the next couple days to get the followup PR in. Thank you!

charkour commented 7 months ago

Looking more at the types, I think this is as good as we are going to get it without breaking API changes. I'll work on a v3 branch with simplification.