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

Does undo/redo merge past state into current state? #196

Open
KevinMusgrave opened this issue Jan 5, 2025 · 5 comments
Open

Does undo/redo merge past state into current state? #196

KevinMusgrave opened this issue Jan 5, 2025 · 5 comments
Assignees
Labels
question Further information is requested

Comments

@KevinMusgrave
Copy link

KevinMusgrave commented Jan 5, 2025

Sometimes I have a zustand state where I add top-level keys dynamically. For example, it might start off like:

useSomething = create(()=>({}))

Then I might later do:

useSomething.setState({[some_id]: some_value})

If undo/redo is merging past states into the current state, then the "some_id" won't be removed.

I'm guessing it is merging, based on this line:

userSet(nextState);

It's not a big deal, as I can just add a permanent top-level key and put the dynamic keys inside of that. Adding an option to overwrite state could be nice though, unless there's a way to do that already.

@charkour
Copy link
Owner

charkour commented Jan 6, 2025

Hey @KevinMusgrave, thanks for the note. I hadn't considered what would happen with dynamic keys. However, I'm surprised that it is not working as expected. If you have time, could you share a Stackblitz reproduction?

The function useSomething.setState() is defined here. The temporal middleware wraps the store's setState with custom logic:

zundo/src/index.ts

Lines 86 to 92 in 65a1c99

store.setState = (...args) => {
// Get most up to date state. The state from the callback might be a partial state.
// The order of the get() and set() calls is important here.
const pastState = options?.partialize?.(get()) || get();
setState(...(args as Parameters<typeof setState>));
temporalHandleSet(pastState);
};

The workaround of one permanent top-level key seems reasonable.

@KevinMusgrave
Copy link
Author

KevinMusgrave commented Jan 6, 2025

Here's a stackblitz project to reproduce: https://stackblitz.com/edit/vitejs-vite-ykmbl1au?file=src%2Fcounter.js

  • main.js sets up the buttons
  • state.js has two states (one with and without a top level key)
  • Clicking on the useSomeState num keys button adds a new key at the top level. Clicking undo doesn't result in a different number of top level keys.
  • Clicking on the useSomeStateNested num keys button adds a new key underneath topLevelKey. Clicking undo works in this case.

Assuming I haven't made a mistake somewhere, the fix would be to call userSet(nextState, true) to make it overwrite state instead of merging.

@charkour
Copy link
Owner

Yes, I think you're right about calling userSet(nextState, true). After taking a closer look, the merge itself it happening with zustand's core. And we would need the replace functionality.

I'll see if there's a way to expose that within zundo.

Thanks for the reproduction.

@charkour
Copy link
Owner

I looked into this a little bit more and have a few follow up questions. One solution is to force a replace every time that a setter in your store is called; however, this only works if you aren't deriving the next key value from the length of keys in the store which isn't a true solution: Stackblitz

So that leaves you with a few options:

  1. Continue to use the topLevelKey workaround. I would consider this to be the most idiomatic zustand solution. When you create a store, you should "know" the fields ahead of time and then use a dynamic data structure as a value.
  2. Update the zundo code to allow for the userSet(nextState, replace) to be set. If I decide to add this feature, would you want replace as an option passed or an argument whenever you call undo/redo? Likely, since there is a work around, I probably won't change zundo but I'd love to hear your thoughts.

@charkour charkour self-assigned this Jan 19, 2025
@charkour charkour added the question Further information is requested label Jan 19, 2025
@KevinMusgrave
Copy link
Author

I think in my particular case, I'd know from the beginning whether I want merging or overwriting, so I'd want it as an option of temporal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants