charkour / zundo

🍜 undo/redo middleware for zustand. <700 bytes
https://codesandbox.io/s/zundo-2dom9
MIT License
597 stars 19 forks source link

The typescript types don't seem to be working correctly #140

Closed charkour closed 8 months ago

charkour commented 8 months ago

This will take some debugging, but it seems like there was a regression with the TS types at some point.

See the type issue with partialize in this code sandbox.

pstrassmann commented 8 months ago

Currently the equality function is typed as:

equality?: (pastState: TState, currentState: TState) => boolean;

However, I think it should instead be typed as

equality?: (pastState: PartialTState, currentState: PartialTState) => boolean;

Since pastState and currentState are always retrieved with the partialized get() like here:

// temporal.ts
const currentState = options?.partialize?.(userGet()) || userGet();

And partialize is typed like:

partialize?: (state: TState) => PartialTState;

When not using partialize, PartialTState will be identical to TState, so this should still be accurate even when users don't use partialize.

I also think it may(?) be helpful to include an explicit example in the documentation of typing when using partialize re useTemporalStore -- specifically that the TemporalState's generic should be PartialTState and not TState when using partialize:

export const useTemporalStore = <T,>(
  selector: (state: TemporalState<PartialTState>) => T,
  equality?: (a: T, b: T) => boolean,
) => useStoreWithEqualityFn(usePageEditorStore.temporal, selector, equality

I don't know if this is the source of the typing issues you are referring to, or if there may be others. But if helpful, I'd be happy to make a PR with these changes. Thank you!

charkour commented 8 months ago

Hey @pstrassmann, thank you for the detailed write-up! I think this is related to the type issues I was seeing on your recent code sandbox.

Would you be willing to make a PR? I would be happy to give a review.

charkour commented 8 months ago

I tested this locally, and I think the only update that needs to be made is in types.ts change which you suggested! Having the docs updated to reflect how to use the hook would be fantastic. If you'd like, you could also make a PR to include the useTemporalStore hook. Thanks!

charkour commented 8 months ago

Fixed in v2.0.3