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

[pstrassmann]

Hello! First, thank you so much for your work on this -- it is amazing!

I noticed that changing the state value of an untracked value prior to changing the state value of a tracked value results in pastStates not getting updated when using handleSet. However, doing this the other way around has no issues (updating the state value of a tracked value prior to updating the state value of an untracked value).

Sandbox: https://codesandbox.io/p/sandbox/zundo-with-partialize-and-deep-equal-xnjwx4?file=%2Fsrc%2FApp.tsx%3A52%2C5

I'm using partialize to not track a certain value.
I'm using handleSet to throttle (with just-throttle)
I'm using equality for deep-equal comparison (fast-deep-equal)

I'm not 100% sure if this is a bug or if I am missing something. Do. you have any insight?

Thank you so much!

[charkour]

Thanks for the kind message and for the reproduction, @pstrassmann!

I'll have to take a look later to determine what's happening. What you're describing sounds like a bug, but I'll need to see what's happening internally when triggering changes. So far, I don't have a good workaround.

[pstrassmann]

@charkour Thank you for taking a look! It is very much appreciated!

I don't know how helpful this is, but in my initial attempts to debug this, the issue appears to possibly be (proximally) caused by the equality function receiving a stale value for newState.

Because the newState is then evaluated to be identical to the pastState, a piece of state history is not added and therefore pastStates is not updated with anything. And this only happens if an untracked value is modified in the store before and in the same callback as a tracked value is modified.

Here is a sandbox with this (ever so slightly further) debugging:

https://codesandbox.io/p/sandbox/zundo-with-partialize-and-deep-equal-debugging-jzgq47?file=%2Fsrc%2FApp.tsx%3A35%2C9

And a code example showing the minor debugging attempt (see equality)

    {
      equality: (pastState, newState) => {
        console.log("pastBears: ", pastState.bears);
        console.log("newBears: ", newState.bears);
        console.log("areBearsEqual: ", pastState.bears === newState.bears); // true

        return deepEqual(pastState, newState);
      },
      handleSet: (handleSet) =>
        throttle<typeof handleSet>((state) => {
          console.log("handleSet called");
          handleSet(state);
        }, 500),
      partialize: (state: MyState): HistoryTrackedState => {
        const { untrackedValue, ...trackedValues } = state;
        return { ...trackedValues };
      },
    },

Thank you again!!

[charkour]

Hi @pstrassmann, I'm working to debug this now. I'm able to reproduce the issue on your sandbox (thank you for creating that!!) and I'll let you know what I find.

[pstrassmann]

@charkour Thank you so so much. I've pulled down the repo and have it linked locally and have been trying to debug a little bit.

I've made a small amount of progress -- I actually think it has nothing to do with partialize (or tracked or untracked values), but instead that the currentState passed to the equality function only reflects the first value that was changed in a callback, if multiple stateSetters were called.

So for example, if this callback is called on a button click:

const handleClick = () => {
   incrementBears()
   incrementCats()
}

the currentState that the equality fn receives only reflects the current, incremented state of bears. currentState.cats remains the same value as in the pastState

See this simplified sandbox if helpful
https://codesandbox.io/p/sandbox/zundo-debugging-v3-ggf3cm?file=%2Fsrc%2FApp.tsx%3A62%2C36

Thank you again.

[charkour]

Thanks, @pstrassmann, that is helpful context! Is it possible to do something like this? Combining the setters into one action could solve the issue. In your latest sandbox, I no longer see the bug, however.

const handleClick = () => {
   incrementBearsAndCars()
}

[charkour]

@pstrassmann, I'm not sure of your usecase in production, but is a valid workaround combining the tracked value and untracked value into one setter function?

I forked the second codesanbox here to add the incrementBoth setter. However, I do think something funny is going on so we'll need to keep looking.

In the most recent Code Sandbox you shared, I don't see the bug.

Thanks for the help on this!

[charkour]

It seems like the beta codesandbox isn't working well... I've tried to update the comment to link here: https://codesandbox.io/p/sandbox/zundo-with-partialize-and-deep-equal-debugging-forked-22v9ms

[pstrassmann]

Hi @charkour -- Thank you for bearing with my as I get a better hold of my issue. Please disregard the last Sandbox I sent you, I was confused. Unfortunately the combined setter doesn't work for us (described more in detail below) due to users being able to modify state in different places in the application all within the throttle interval.

The TL;DR of my issue is: Is there a way to conditionally trigger a throttle depending on which fields of the zustand store
change? If so, would it require handleSet callback to have access to pastState and currentState?

More detail:

To summarize, the issue I'm having is that when using both partialize and handleSet to implement a throttle, changing the state of an untrackedValue in zustand triggers the throttle, making it so that any subsequent state setting of tracked values (that fall within the throttle window) to not be registered. I now realizse this isn't a bug -- throttle is working exactly as intended by running on every change to the zustand store. And because a change to the store could contain changes to both untracked and tracked values (eg theoretically something like incrementUntrackedValueAndBears()), this makes sense as the way it would work.

However, in our application, users may perform actions in various places that change untracked and tracked values, making it unrealistic to combine these actions into a single state setter as you suggested like incrementUntrackedValueAndBears()

As a contrived example, take two buttons

const handleButtonClickA = () => {
   incrementUntrackedValue()
}

const handleButtonClickB = () => {
   incrementTrackedValue()
}

Sandbox for above example: https://codesandbox.io/p/sandbox/zundo-untrackedvalue-wq9cm8?file=%2Fsrc%2FApp.tsx%3A63%2C11

If ButtonA is clicked and then ButtonB is clicked all within the throttle interval, the change to the trackedValue will not be registered in history, as ButtonA initiated the throttle interval.

A potential solution to my problem could be to make throttle smarter -- if a state setting action only changes zustand store fields that are untracked, throttle interval should not be initiated. If I could do something like that, then:

// This would never initiate the throttle interval
const handleButtonClickA = () => {
   incrementUntrackedValue()
}

// If clicked immediately after ButtonA, this would be registered in history and throttle interval initiated.
const handleButtonClickB = () => {
   incrementTrackedValue()
}

I'm still chewing on this one, but if you have any insight for how to do this -- or a potential change to a forked version of zundo, it would be much appreciated!

To achieve this, I'm wondering if the handleSet callback would need access to both pastState and currentState, diffing the two, and checking whether the diff only contains changes to untracked fields. Is something like this within the realm of possibility? I feel like other users with relatively complex applications could also encounter this issue, given that zustand appears to be biased towards structuring things as a single store? But I'm not sure.

Thanks so much for taking the time to help with this!

[pstrassmann]

Hi @charkour . I think I've identified a fix to the issue -- I included the details in a PR. If you could take a look it would be most appreciated!

The general idea is:

Currently handleSet is called even if (history tracked) state hasn't changed. That is, handleSet is called after any change to the zustand store. This PR changes this behavior so that handleSet is only called if history tracked state has changed.

For consistency, the if block that I added in index.ts was copied directly (syntax included) from temporal.ts. Let me know if you think this is behavior that makes sense to include in zundo, or potentially to include as a configurable option. Thank you again for taking a look!

[charkour]

Hey @pstrassmann, thank you for the detailed work on this! I'll take a look at the PR. Thanks again.

For the time being, do you have a suitable workaround?

[pstrassmann]

Thank you for taking a look! Unfortunately we don’t have an alternative work around at the moment that uses ‘zundo’ (but we also don’t have an immediate alternative without ‘zundo’, without forking).

I’ll keep working on this however — appreciate all the help!

[charkour]

Sounds good, for the time being, you could use something like patch-package (if you're using npm). Yarn and Pnpm have other patching solutions.

[pstrassmann]

Ah that’s wonderful, thank you for that!

[charkour]

Is patching the package locally a suitable workaround for now as we determine the best way to fix this issue? Thank you

[pstrassmann]

I'll work on patching the package locally today. I believe patching should be a suitable workaround in the short term, thank you for that! In the meantime, I'm happy to address any feedback big or small related to either of the potential solutions in the PRs.

Also, if you'd prefer to implement an alternative solution yourself or reimplement one of my solutions to be more in line with your stylistic preferences, that is of course also okay! Take whatever time you need to feel comfortable with a solution.

Thank you for your help!

[charkour]

Thanks for being flexible and willing to work on solutions! I wish I could work on this full time haha

Are you using zundo in prod or in a personal project?

[pstrassmann]

We are looking at adding zundo in prod to a currently live application at Classy!

[pstrassmann]

Just adding some additional thoughts about the solutions for potentially helpful context during review:

• PR 141 assumes that users will never want their user-provided handleSet to be called if it is true that state history will not update. That is, there should never be a situation in which a cool-off is triggered when state history is not going to get updated. I think this is a fair assumption, but it is at least conceivable (although I'm not sure for what purpose) that a user might want changes to entirely untracked values to trigger a cool-off. However, at the very least I think this behavior should be configurable somehow by the user, as it seems more likely that users would only want changes to state history to actually trigger the cool-off.

• PR 142 gives maximum flexibility back to the user, but a potential(?) drawback is that it could make the API seem slightly more complex.

For example, with PR 142, the user would not need to provide a diff nor equality fn if they write their handleSet to only trigger according to an equality fn within handleSet and they wanted cool-off and state history setting to be in sync. For example:

      handleSet: (handleSet) => {
          const throttleFn = throttle((state) => {
              handleSet(state); }, 1000, );

          return ( pastState, currentState ) => {
            if (isDeepEqual(pastState, currentState) {
              throttleFn(pastState);
            }
          };

With the above, the cool-off and state history setting always remain in sync (assuming no additional diff or equality fn), because state setting runs on every successful call of handleSet within the throttle fn.

However, if the user additionally provided an equality fn ZundoOption, whether handleSet within the throttle is called is determined by isDeepEqual (or whatever a user writes) within the user handleSet, and whether state history is added to would then be determined by the equality fn here. Although this could be considered a feature, I would want to make sure that this didn't feel like it was overly complicating the API, particularly if the demand for such a distinction in cool-off vs state-history setting seemed unlikely.

[charkour]

I'm glad you're considering using this in production! It's nice to see the ecosystem growing around small but performant state solutions.

Thanks for the additional context; I'm currently testing out some potential work around, but I'm leaning towards merging in PR #141 to avoid an overly complicated API as you stated. Thank you for the great help on this!

[charkour]

To close the loop, what you reported is a bug (and oversight in my initial implementation) that can be fixed via #141. Thanks for reporting this!

[pstrassmann]

Thanks @charkour! I made some minor changes based on your feedback to PR 141. The main new change is to dry up the conditional calling of curriedHandleSet based on equality/diff functions and make sure it is being handled identically in both ways of setting zustand state, as you helped me understand.

After some noodling, I do not currently think it is possible to pass additional parameters to curriedHandleSet without adding some minor changes to the API (handleSet needs to be able to receive more parameters to be able to receive more parameters 🥴). Although I think the downsides of not being able to do this are minor, there is likely a way to do this that would require a more major refactor of how zundo works (I only have vague fuzzy maybe fake ideas).

However, I also don't think that the API changes required to be able to pass additional paramaters to curriedHandleSet are particularly significant (they would be non breaking I think, and I don't think users would need to refactor any code at all). So I demo'd what that would look like in an additional PR here. I am not sure though what changes require majors version bumps and which do not. Anyways, that PR is sort of a hybrid of the two other PRs -- if you happen to like it better.

I know it's the holidays so no rush and please take your time, and thanks again for your help with all of this!

[charkour]

Hey @pstrassmann, thank you for the awesome work on both the bug write-up and creating multiple PRs. This is what makes open source so awesome. I hope you had a nice holiday too.

As for version changes, I recommend checking out the Semver docs. In short, if we are adding features (such a more parameters in a callback) then it's a minor version change, but if we remove or change parameters or the external behavior, then it's a major version change. For small fixes, it would be a patch. I have my gripes with semver (I wish it were at least four numbers, not three), but it's what the industry uses :)

Thanks for being conscious of avoiding breaking changes! Happy to chat more about this if you have more questions. Thank you

[charkour]

This has been resolved in #149

I'll release v2.1.0 shortly.

[charkour]

This was fixed in v2.1.0