For my understanding, these lines here are a more verbose way to write these four lines, correct?

   const curriedHandleSet =
      options?.handleSet?.(
        (store.temporal.getState() as _TemporalState<TState>)._handleSet,
      ) || (store.temporal.getState() as _TemporalState<TState>)._handleSet;

If so, could we revert these changes to cut down on the number of lines changed? Thanks!

Should this if statement also be moved into temporalHandleSet()?

Taking a look at this now, I'm wondering if _handleSet should simply be removed from the temporal store because it's never externally referenced. What do you think?

Re removing _handleSet -- my apologies, I am not totally confident I understand the potential refactor.

Although _handleSet shouldn't be accessed by the user (it is notably omitted from the TemporalState type, along with _onSave), _handleSet does still need access to get and set params passed by createStore to the stateCreator function returned by temporalStateCreator.

And then _handleSet is accessed in the curriedHandleSet expression via (store.temporal.getState() as _TemporalState<TState>)._handleSet;

I'm just trying to puzzle through how _handleSet might otherwise have access to get and set of the temporal store

EDIT: thinking more about this, I think I am understanding. We could remove it from the temporal store and just use store.temporal.getState() and store.temporal.setState

vaguely like:

// index.ts, below where we add `store.temporal = createStore...`

const handleSet = (pastState, currentState, deltaState) => {
      if (store.temporal.getState().isTracking) {
        // This naively assumes that only one new state can be added at a time
        if (options?.limit && store.temporal.getState().pastStates.length >= options?.limit) {
          store.temporal.getState().pastStates.shift();
        }

        store.temporal.getState()._onSave?.(pastState, currentState);
        store.temporal.setState({
          ...store.temporal.getState(),
          pastStates: get().pastStates.concat(deltaState || pastState),
          futureStates: [],
        });
      }
    }

Is this what you had in mind?

Re moving that if statement, yes that makes sense!

Yes! Your code snippet is what I had in mind; however, this is simply a hunch that it will work but I haven't tested it.

We could leave it as a follow-up PR to avoid many changes in a single PR. What do you think about you or I making this change after this current PR is merged?

Yeah that sounds good! Happy to make a follow up PR after this one to test it out.

My only concern here is that this is technically a breaking type change. Have you considered (this mess of a) type signature to avoid breaking type changes?

I had not previously considered this, but if it is okay with you then it is okay with me! Very cleverly avoids the breaking change!

I doubt that users will ever want to access the value of replace in the function returned by their handleSet function, but they already can as is. I will update to reflect this on my next push. I'll plan on passing undefined as the value for "replace" given to curriedHandleSet, which is what the value is now.

Let's plan on this to avoid a breaking change. Can you please add a comment in the future replace will likely be deprecated and removed? Thanks!

I have some concerns that this type definition may spook new users reading the README, but it also seems like a good idea to be accurate here?

I'm wondering if there are easier ways to write this out. If not, it is what it is and we can simplify this in v3.

I'm not sure. The use of the Parameters utility type is actually easier on the eyes to me than directly using/copying/extending the types from SetStateInternal from zustand:

type SetStateInternal<T> = {
    _(partial: T | Partial<T> | {
        _(state: T): T | Partial<T>;
    }['_'], replace?: boolean | undefined): void;
}['_'];

I think the addition in this PR just looks ugly (in part?) because, whereas previously replace was hidden within StoreApi<TState>['setState'], we make it visible here in order to add the two additional parameters currentState and deltaState

I agree. Let's merge this in as is, but I'll make a follow-up PR with some options.

Why is this typecase needed? Is that because this function technically takes four parameters including currentState and deltaState?

Yeah that is right. I believe it is required due to the fact that curriedHandleSet is used to call either the internal handle set or the user-provided handleSet.

Perhaps this is a circular explanation, but options.handleSet takes an argument of type StoreApi<TState>['setState'].
However, this is not the type of _handleSet, which takes four arguments. So asserting it to the be the type expected by options.handleSet resolves the type error.

I'll be the first to admit that this part of the code is not the easiest to read/follow. I don't immediately have an alternative suggestion, but I would certainly be open to something more approachable in upcoming versions.