fix equality fn type and update docs by pstrassmann · Pull Request #144 · charkour/zundo

pstrassmann · 2023-12-22T18:41:45Z

This PR updates the types as mentioned here.

I also included some updates to the docs. If you'd prefer I separate these docs updates into a separate PR, I'm happy to do so!

When I was first reading the docs for zundo there were some points of initial confusion for me, so some of my suggested updates are to clarify those things.

Thank you!

pstrassmann · 2023-12-22T18:51:50Z

README.md

+  const { bears, increasePopulation, removeAllBears } = useStoreWithUndo();
+  // See API section for temporal.getState() for all functions and
+  // properties provided by `temporal`
+  const { undo, redo, clear } = useStoreWithUndo.temporal.getState();


This may be personal preference, but I think it is simpler in the docs to introduce (and recommend) useStore.temporal.getState() prior to something like useTemporalStore. I think most users will not need to access member properties of the temporal object in a reactive way (that is, they'll likely just need undo, redo, clear and not need reactive access to pastStates or currentStates, isTracking, etc. I think introducing it this way may help facilitate a more straightforward mental model of what zundo is doing. But I'm not sure!

I agree with this, thank you for making this change and for providing an explanation!

pstrassmann · 2023-12-22T18:53:44Z

README.md

-## Then bind your components
-
-Use your store anywhere, including `undo`, `redo`, and `clear`!
+) => useStore(useStoreWithUndo.temporal, selector, equality);


For consistency in the docs, I replaced all references to stores created with temporal with useStoreWithUndo. I think this could makes things more clear to readers, instead of referring to the store by multiple names like useStoreWithUndo, originalStore, useStoreA, withTemporal, etc.

Thanks, I didn't realize I used a different name each time. Oops!

pstrassmann · 2023-12-22T18:55:55Z

README.md

 );
 ```

+If converting temporal store to a React Store Hook with typescript, be sure to define the type of


This is new -- giving an example of useTemporalStore with partialized state. If users useTemporalState<StoreState> while using partialize as a ZundoOption, they will see a type error. So this is to preempt that kind of error by providing an example of useTemporalState with a partalized store state typescript generic

pstrassmann · 2023-12-22T19:01:29Z

README.md

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

-For performance reasons, you may want to use a custom `equality` function to determine when a state change should be tracked. You can write your own or use something like [`fast-equals`](https://github.com/planttheidea/fast-equals), [`fast-deep-equal`](https://github.com/epoberezkin/fast-deep-equal), [`zustand/shallow`](https://github.com/pmndrs/zustand/blob/main/src/shallow.ts), [`lodash.isequal`](https://www.npmjs.com/package/lodash.isequal), or [`underscore.isEqual`](https://github.com/jashkenas/underscore/blob/master/modules/isEqual.js). By default, all state changes to your store are tracked.
+Provide an `equality` function to determine when a state change should be tracked.


I removed "performance reasons", as I think many reasons to use a custom equality function are not actually for performance reasons (although some are!). I think many instances of using an equality function are to ensure that effectively duplicate state histories are not tracked. For example, by using deep-equality or shallow equality fns. My impression is that the performance considerations may have more to do with which equality fn one chooses, as without one, history is naively added to no matter if shallow or deep equality is true.

Thanks for the explanation! Could you find a way to include possible reasons someone might use this option? I included performance originally because people asked for that reason and I wanted people to Cmd+F performance in the readme. However, I agree there are other important reasons too, so could you include more of them? Thank you!

No problem! I expanded the section a little bit, and added back the word performance for easy Cmd+F-ing -- let me know fi you think it is clear or if I should try to expand it more. I'm trying to find the right balance of not bloating the code examples while also trying to be as immediately clear to the reader as possible. If you think it would be even clearer to provide example StoreState interfaces for the various use-cases, I'm happy to add those. But if it seems to be sufficient without them, I think my instinct is to try to stay as lean as possible.

Thanks for making that change! Striking the balance of keeping docs clear but bloated is difficult and I appreciate you for trying to find that balance too.

pstrassmann · 2023-12-22T19:02:55Z

README.md

 Sometimes multiple state changes might happen in a short amount of time and you only want to store one change in history. To do so, we can utilize the `handleSet` callback to set a timeout to prevent new changes from being stored in history. This can be used with something like [`throttle-debounce`](https://github.com/niksy/throttle-debounce), [`just-throttle`](https://github.com/angus-c/just/tree/master/packages/function-throttle), [`just-debounce-it`](https://github.com/angus-c/just/tree/master/packages/function-debounce), [`lodash.throttle`](https://www.npmjs.com/package/lodash.throttle), or [`lodash.debounce`](https://www.npmjs.com/package/lodash.debounce). This a way to provide middleware to the temporal store's setter function.

 ```tsx
-const withTemporal = temporal<MyState>(


It was initially confusing for me that these examples did not use create from zustand. I think it makes sense to keep a common code template for all ZundoOptions API examples, so this was updated here.

pstrassmann · 2023-12-22T19:03:40Z

README.md


 > While `setState`, `subscribe`, and `destroy` exist on `temporal`, you should not need to use them.

-#### **React Hooks**


This was removed, as it seemed to be a duplicate of useTemporalStore example earlier in the docs

charkour · 2023-12-22T22:46:29Z

Overall these documentation updates are looking great. I'll give my complete review tomorrow. Thanks!

charkour

These updates are fantastic, thank you!

charkour · 2023-12-23T16:03:24Z

README.md

+`zundo` has one export: `temporal`. It is used as middleware for `create` from zustand. The `config` parameter is your store created by zustand. The second `options` param is optional and has the following API.

-### Middleware Options
+### Bear's eye view:


This little easter egg is great!

Haha nice! I was hoping you'd like it 😆

pstrassmann · 2023-12-23T16:44:42Z

Thank you for reviewing and bringing in these changes!

pstrassmann-gfm added 2 commits December 21, 2023 13:28

fix: equality fn types

61a5bc7

update: docs

db729dc

pstrassmann commented Dec 22, 2023

View reviewed changes

charkour self-assigned this Dec 22, 2023

pstrassmann commented Dec 22, 2023

View reviewed changes

fix: wrong type pick in docs

a3d7b3a

pstrassmann-gfm and others added 2 commits December 22, 2023 20:38

docs: expand on equality fn

1791df6

Merge branch 'main' into pstrassmann-types-fix

73fb9d4

charkour approved these changes Dec 23, 2023

View reviewed changes

charkour merged commit 52f90e7 into charkour:main Dec 23, 2023

charkour reviewed Dec 23, 2023

View reviewed changes


		> While `setState`, `subscribe`, and `destroy` exist on `temporal`, you should not need to use them.

		#### React Hooks

Uh oh!

Comments

Conversation

pstrassmann commented Dec 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pstrassmann Dec 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charkour commented Dec 22, 2023

Uh oh!

charkour left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pstrassmann commented Dec 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pstrassmann Dec 22, 2023 •

edited

Loading