fix: don't call handleSet if state hasn't changed by pstrassmann · Pull Request #141 · charkour/zundo

pstrassmann · 2023-12-15T00:16:45Z

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:

charkour · 2023-12-15T01:08:43Z

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 · 2023-12-15T01:25:27Z

@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.

pstrassmann · 2023-12-15T19:47:57Z

tests/__tests__/options.test.ts

      expect(console.warn).toHaveBeenCalledTimes(2);
    });
+
+    it('should not call throttle function if partialized state is unchanged according to equality fn', () => {


This test fails on main branch

pstrassmann · 2023-12-15T19:48:09Z

tests/__tests__/options.test.ts

+      vi.useRealTimers();
+    });
+
+    it('should not call throttle function if partialized state is unchanged according to diff fn', () => {


This test fails on main branch

pstrassmann · 2023-12-15T19:53:26Z

tests/__tests__/options.test.ts

+      vi.useRealTimers();
+    });
+
+    it('should always call throttle function on any partialized or non-partialized state change if no equality or diff fn is provided', () => {


This test passes on main branch, but seemed helpful to explicitly define and to call out.

See this gif for what this looks like (notice useless history):

Sandbox: https://codesandbox.io/p/sandbox/zundo-handleset-partialize-with-no-equality-fn-and-no-diff-fn-zvs3sx?file=%2Fsrc%2FApp.tsx%3A23%2C37

Gif:

charkour · 2023-12-23T22:11:02Z

src/index.ts

+            )
+          )
+        ) {
+          curriedHandleSet(pastState);


If we go forward with this, what do you think about passing currentState and deltaState to the curriedHandleSet?

Suggested change

curriedHandleSet(pastState);

curriedHandleSet(pastState, currentState, deltaState);

Yeah this makes sense!

@charkour Actually I think we may run into some type issues doing this if we want to use a curriedHandleSet rather than something more like in PR: 142. I'm not positive and am trying to troubleshoot this now.

Expanding handleSet to expect additional params currentState and deltaState is currently causing some tangles, where we would need to know currentState and deltaState when we define curriedHandleState, which I'm not sure is possible (since it is defined outside of any function that sets zustand state so that currentState becomes a thing)

I'll keep thinking about it.

Ah, that is tricky, maybe this way isn't doable without a major version change. Could you see what's possible to do without major version changes and then we could consider a future API for v3?

Yeah for sure, I'll keep noodling on this and see if I can find a solution that is more of something in between PR 141 and 142, biasing toward something that doesn't change the API

Sounds good! Thanks for the help on this!

charkour · 2023-12-23T22:19:09Z

src/temporal.ts

        if (get().isTracking) {
          const currentState = options?.partialize?.(userGet()) || userGet();
          const deltaState = options?.diff?.(pastState, currentState);


Could you move these lines into the src/index.ts file?

charkour · 2023-12-23T22:20:33Z

src/index.ts

+              options?.equality?.(pastState, currentState) ||
+              // If the user has provided a diff function but nothing has been changed, function returns null
+              deltaState === null


Looking back at the old code, an improved heuristic would be to first check deltaState === null before checking equality because that could be a potentially expensive operation.

Please make that change here.

charkour · 2023-12-23T22:22:21Z

src/index.ts

        const pastState = options?.partialize?.(get()) || get();
        set(...args);
-        curriedHandleSet(pastState);
+        const currentState = options?.partialize?.(get()) || get();


For consistency, could you also make sure this change takes place in the setState function on line 58? Thanks!

Yeah can do. To be honest I haven't quite been able to wrap my head around what is going on on line 60. Do you happen to have a quick way of explaining what is going on there? 😅

Sure! The quick explanation is that zustand has two ways of setting state; one is during the creation method where set is used within the callback (used often) and the second is after the store is created, calling myStore.setState (which is less used):

const myStore = create((set) => ({ increment: set((state) => ({ bears: state.bears + 1 })}); myStore.setState({ bears: 99 });

The zundo user might prefer one setter over the other, but to make sure the middleware acts the same, we need to modify both setters. This was really brief, so I'm happy to elaborate more.

The set version is updated to track history within the config function call, and the setState version is directly mutated on the user's store object (the object returned by zustand/create or zustand/createStore).

Beautiful, thank you!

charkour · 2024-01-14T14:56:41Z

Closing in favor of #149

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

0da723a

pstrassmann mentioned this pull request Dec 15, 2023

Updating the value of an untracked value prior to updating a tracked value results in pastStates not updating when using handleSet (Sandbox example) #139

Closed

pstrassmann-gfm added 2 commits December 15, 2023 11:41

fix: add tests

805df96

fix: minor cleanup

6e80482

pstrassmann commented Dec 15, 2023

View reviewed changes

pstrassmann mentioned this pull request Dec 16, 2023

Give users more flexibility in writing handleSet fn (alternative solution to PR #141) #142

Closed

charkour self-assigned this Dec 23, 2023

charkour reviewed Dec 23, 2023

View reviewed changes

refactor: be consistent between state setting methods and dry code

b7a0b76

pstrassmann mentioned this pull request Dec 24, 2023

handleSet refactor option 3 #149

Merged

charkour closed this Jan 14, 2024

	curriedHandleSet(pastState);
	curriedHandleSet(pastState, currentState, deltaState);

Uh oh!

Comments

Conversation

pstrassmann commented Dec 15, 2023

Uh oh!

charkour commented Dec 15, 2023

Uh oh!

pstrassmann commented Dec 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pstrassmann Dec 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pstrassmann Dec 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charkour Dec 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charkour Dec 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charkour commented Jan 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pstrassmann commented Dec 15, 2023 •

edited

Loading

pstrassmann Dec 23, 2023 •

edited

Loading

pstrassmann Dec 23, 2023 •

edited

Loading

charkour Dec 23, 2023 •

edited

Loading

charkour Dec 23, 2023 •

edited

Loading