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 · 2 comments
Open

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

KevinMusgrave opened this issue Jan 5, 2025 · 2 comments

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.

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

No branches or pull requests

2 participants