charkour / zundo

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

fix: don't call handleSet if state hasn't changed #141

Closed pstrassmann closed 7 months ago

pstrassmann commented 8 months ago

Currently, handleSet gets called even if temporal state hasn't changed. One negative result of this is that throttles or debouncers to tracking history can get initialized even if no history-tracked state changes. This can result in unexpected behavior, such as the first instance of a history tracked state changing not getting registered in history, so long as it was preceded by a non-history tracked state changing.

This PR implements a change so that handleState is only called if pastState and currentState are not equal.

I'd appreciate some review and feedback, as I am new to this repo and want to make sure that I'm handling all edge cases correctly.

This PR has been tested locally against code like in this Sandbox: https://codesandbox.io/p/sandbox/zundo-untrackedvalue-change-initiating-throttle-issue-wq9cm8?file=%2Fsrc%2FApp.tsx%3A58%2C11

Here is a gif of this fix working: zundo

charkour commented 8 months ago

Hi @pstrassmann, thanks for the PR! Could you write tests that fail but pass with your fix?

I realize the bug is due to the curriedHandleSet being wrapped with options.handleSet in index.ts. The curriedHandleSet is wrapped by the throttle function. Your change moves the equality check to before the throttling, which fixes your issue. I'm wondering if this will cause regressions elsewhere or if this fixes bugs. Adding tests cases will help us determine that. Thank you!

pstrassmann commented 8 months ago

@charkour Yes, will do. I am also concerned with causing regressions or inadvertently introducing a breaking change.

I also have one other idea that Iā€™ll write write up tomorrow that may be more in the zundo spirit of leaving implantation to the user.

The basic idea would be to allow the user access ā€˜previousStateā€™ and ā€˜currentStateā€™ inside the callback that consumers pass to the ā€˜handleStateā€™ ā€˜ZundoOptionā€™. With access to previous and current state, they could themselves then write the conditional that determines whether ā€˜handleStateā€™ is run. But there may be snags I havenā€™t thought of, but I will keep chewing on it.

charkour commented 7 months ago

Closing in favor of #149