Add persist options for temporal store by SugarF0x · Pull Request #61 · charkour/zundo

SugarF0x · 2023-01-08T14:15:24Z

Expose internal temporal store state creator function to allow for middleware injections which include but are not limited to persist

Todo:

Cleanup
Fix tests

This reverts commit d2eaef0.

This reverts commit 5a9179f.

charkour · 2023-01-10T19:41:17Z

Thanks for this! @SugarF0x

I'll help update the tests. Would you like me to make a PR in your fork, is it okay to push to this branch (you might have to click "allow edits from maintainers".

SugarF0x · 2023-01-10T21:12:59Z

@charkour Aye, ive checked maintainers' edits so you can push straight to my fork's main branch

charkour · 2023-01-11T04:54:20Z

Sorry, there was a bug due to the lastest version of zustand. Would you be willing to update this PR?

# Conflicts: # packages/zundo/__tests__/createVanillaTemporal.test.ts # packages/zundo/__tests__/options.test.ts # packages/zundo/__tests__/zundo.test.ts # packages/zundo/src/index.ts # packages/zundo/src/temporal.ts # packages/zundo/src/types.ts # pnpm-lock.yaml

SugarF0x · 2023-01-14T09:24:07Z

@charkour aye, fixed that up nicely, working just as fine 👌

SugarF0x · 2023-01-14T17:10:42Z

@charkour reconsidered approach, allowed use of practically any kind and any amount of temporal store middlewares while containing the logic within the temporal middleware itself 👌
now just gotta figure out a proper type cast for it 🤔

charkour · 2023-01-14T21:02:20Z

Awesome! I'll take a look at it now. Thanks for the work on it!

charkour · 2023-01-14T21:17:49Z

@SugarF0x, do you like the ability to add different middleware to the temporal store than the main store?

Originally, I thought that we could add the temporal state to the main store. What do you think? Maybe both should be an option.

SugarF0x · 2023-01-14T21:40:56Z

@charkour we could add an option to toggle between the in-store and out-of-store options to base the temporal store at

Initially i also thought about adding temporal to the main store but then decided that this would be a bit of a breaking change

Plus, Im not too convinced that tampering with one's store directly is not too user friendly since that happens implicitly

Imagine adding an undo function to the parent store directly where user has already had an undo function declared effectively overriding it making one question why their function does not what they want it to

Change merge order and now we have a different issue - user declared undo function overrides temporal undo function making it impossible to call temporal undo at all

While that could be pointed out in the README as a caveat or all the temporal functionality could be shoved under a _temporal key at the store root level, but is it really that much better?

I dont think either option is inherently good or bad, Im just trying to play devils advocate here

Both ideas are good and the final choice is on you as the author

In the end, i only ever stuck with the current implementation of having temporal in a dedicated store purely out of the backwards-compatibility consideration

charkour · 2023-01-14T22:09:25Z

I agree with you and your line of thinking. Let's go forward with keeping the temporal state separate from the user state, but I'll also add a separate function that can be used to inject the temporal state into the user state to make middleware simpler.

charkour · 2023-01-14T22:34:12Z

@SugarF0x, would you be willing to update the tests for this PR? I can't seem to be able to edit this branch.

I'm working on the function that allows you to inject directly into the user's state.

SugarF0x · 2023-01-15T08:42:31Z

weird 🤔

coming back to the backwards compatibility, no prior tests break 👐

charkour · 2023-01-15T17:16:18Z

Tests are looking good!

Thanks for leading this feature development! I think once this is merged, we can release v2 finally!

I'll check this over in a couple hours.

charkour

A couple quick comments

packages/zundo/src/temporal.ts

apps/web/pages/subscribeWithSelector.tsx

charkour · 2023-01-15T17:21:27Z

packages/zundo/src/types.ts

-    handleSet: StoreApi<TState>['setState'],
-  ) => StoreApi<TState>['setState'];
+  handleSet?: (handleSet: StoreApi<TState>['setState']) => StoreApi<TState>['setState'];
+  storeWrap?: (initializer: StateCreator<TemporalStateWithInternals<TState>, [], []>) => StateCreator<TemporalStateWithInternals<TState>, [], []>


What other names did you think of for storeWrap? I'm wondering if there is a better one we could use.

Perhaps, temporalMiddleware or simply middleware? Maybe storeMiddleware or wrapTemporalStore?

I'm not sure if there are other examples is in the zustand ecosystem that use this pattern we could copy.

What do you think?

storeWrap was more of a WIP TBD name, either middlewares or middlewareInjection would be much more suitable

alternatively, this could be called storeInitializerInjection since the purpose if it is to add some custom functionality e.g. middlewares to a store initializator which is the zustand name for the function we pass to create method

Okay, I prefer the name wrapTemporal or wrapTemporalStore.

What do you think?

Once these small comments are addressed, we can merge this in, add docs, and release a new version! Thank you for all the great help.

Happy to be working on this, amazing opportunity to both learn more about middlewares as well as to do some good for the people using this much like i do

wrapTemporalStore sounds good to me

charkour · 2023-01-15T17:29:31Z

packages/zundo/src/index.ts

 };

 export const temporal = zundoImpl as unknown as Zundo;
+export { TemporalStateWithInternals };


Is this export needed?

ideally we should not be exposing that, its currently sitting here for testing purposes

charkour

This is looking good! Thanks for this.

Once this is no longer a draft, we can merge it in!

SugarF0x · 2023-01-16T07:02:51Z

ye i just gotta figure out how to fix the typing system
its quite complicated with how zustand handles middleware state interface merging
all looks fine on paper but in practice the types go haywire 🤯

charkour · 2023-01-16T13:05:26Z

Sounds good! The types stretch my understanding as well.

I'll look into the type errors as well.

SugarF0x · 2023-01-17T21:31:57Z

@charkour i fixed the typings
does it work? yes
is it pretty? debatable

SugarF0x · 2023-01-17T21:40:15Z

decided to check how other middleware libraries handle middleware injection

💀

charkour · 2023-01-17T21:47:27Z

Oh wow. Not much type safety there.

charkour · 2023-01-17T21:48:21Z

Also, the typings are looking good! Thanks again for working on it.

charkour · 2023-01-19T13:06:59Z

Would you like a review on this? Or is it still a draft?

SugarF0x · 2023-01-19T13:28:39Z

@charkour ye i think its review worthy at this point
i wanna give the tests another look this weekend but the implementation itself is pretty much ready i believe

# Conflicts: # packages/zundo/src/temporal.ts

… the stuped

charkour · 2023-01-21T20:29:27Z

packages/zundo/vitest.config.ts

+export default defineConfig({
+  test: {
+    globals: true, // required
+    setupFiles: ['vitest-localstorage-mock'],


This is a nice package. Helpful for testing. Thanks!

# Conflicts: # apps/web/package.json # packages/zundo/package.json # packages/zundo/src/index.ts # pnpm-lock.yaml

charkour · 2023-02-20T14:18:03Z

Thanks for the updates to the branch!

SugarF0x · 2023-02-20T14:38:03Z

@charkour aye, my pleasure
is there any eta on this going through?

charkour · 2023-02-20T14:48:28Z

Yeah, sorry this is taking so long. Let's say one week. That way I'll prioritize reviewing this.

Thank you so much for your effort and patience.

charkour · 2023-02-28T03:43:30Z

I'll merge this into the rc-2 branch to prep the Release Candidate for v2

charkour · 2023-02-28T03:43:43Z

Thanks for your patience on this.

SugarF0x added 7 commits January 7, 2023 22:38

proof of concept for in-state temporal store

5a9179f

fix missing temporal functions after persistence merge

d2eaef0

Revert "fix missing temporal functions after persistence merge"

fe96f50

This reverts commit d2eaef0.

Revert "proof of concept for in-state temporal store"

490d5cd

This reverts commit 5a9179f.

disable tests

b418a0c

working prototype

e74d0a2

fix persistence merge issue

15e738b

SugarF0x mentioned this pull request Jan 8, 2023

Will this work with native persist middleware? #45

Closed

charkour added the enhancement New feature or request label Jan 10, 2023

SugarF0x added 2 commits January 14, 2023 12:18

disable tests again

6e5c7e9

rewrite logic to allow all kinds on middlewares

02ad4bf

SugarF0x added 2 commits January 15, 2023 11:42

reenable tests

3e87ee1

add storeWrap option test

bce8880

charkour reviewed Jan 15, 2023

View reviewed changes

SugarF0x added 2 commits January 15, 2023 21:03

rename injector to store wrapper

c3ad9ef

uncomment selector subscriber

e94008d

charkour approved these changes Jan 15, 2023

View reviewed changes

fix typings

4ee286d

SugarF0x marked this pull request as ready for review January 19, 2023 13:28

SugarF0x requested a review from charkour January 19, 2023 13:29

SugarF0x changed the title ~~[WIP] Add persist options for temporal store~~ Add persist options for temporal store Jan 19, 2023

SugarF0x added 4 commits January 19, 2023 22:25

Merge branch 'main-source'

340cab0

# Conflicts: # packages/zundo/src/temporal.ts

turns out explicit manual type case was only necessary because i have…

7a22f1d

… the stuped

fix typing again 💢

dbb0f1d

remove unnecessary option from tests

e5b1786

charkour self-assigned this Jan 20, 2023

charkour reviewed Jan 21, 2023

View reviewed changes

SugarF0x added 3 commits January 22, 2023 13:28

remove temporal internals type export

e301289

add zundo options to exports

1a96cc3

Merge branch 'main-source'

c679dc6

# Conflicts: # apps/web/package.json # packages/zundo/package.json # packages/zundo/src/index.ts # pnpm-lock.yaml

charkour changed the base branch from main to rc-2 February 28, 2023 03:43

charkour merged commit 2399917 into charkour:rc-2 Feb 28, 2023

Uh oh!

Comments

Conversation

SugarF0x commented Jan 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Expose internal temporal store state creator function to allow for middleware injections which include but are not limited to persist

Uh oh!

charkour commented Jan 10, 2023

Uh oh!

SugarF0x commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

charkour commented Jan 11, 2023

Uh oh!

SugarF0x commented Jan 14, 2023

Uh oh!

SugarF0x commented Jan 14, 2023

Uh oh!

charkour commented Jan 14, 2023

Uh oh!

charkour commented Jan 14, 2023

Uh oh!

SugarF0x commented Jan 14, 2023

Uh oh!

charkour commented Jan 14, 2023

Uh oh!

charkour commented Jan 14, 2023

Uh oh!

SugarF0x commented Jan 15, 2023

Uh oh!

charkour commented Jan 15, 2023

Uh oh!

charkour left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

charkour Jan 15, 2023

Choose a reason for hiding this comment

Uh oh!

SugarF0x Jan 15, 2023

Choose a reason for hiding this comment

Uh oh!

charkour Jan 15, 2023

Choose a reason for hiding this comment

Uh oh!

SugarF0x Jan 15, 2023

Choose a reason for hiding this comment

Uh oh!

charkour Jan 15, 2023

Choose a reason for hiding this comment

Uh oh!

SugarF0x Jan 15, 2023

Choose a reason for hiding this comment

Uh oh!

charkour left a comment

Choose a reason for hiding this comment

Uh oh!

SugarF0x commented Jan 16, 2023

Uh oh!

charkour commented Jan 16, 2023

Uh oh!

SugarF0x commented Jan 17, 2023

Uh oh!

SugarF0x commented Jan 17, 2023

Uh oh!

charkour commented Jan 17, 2023

Uh oh!

charkour commented Jan 17, 2023

Uh oh!

charkour commented Jan 19, 2023

Uh oh!

SugarF0x commented Jan 19, 2023

Uh oh!

charkour Jan 21, 2023

Choose a reason for hiding this comment

Uh oh!

charkour commented Feb 20, 2023

Uh oh!

SugarF0x commented Feb 20, 2023

SugarF0x commented Jan 8, 2023 •

edited

Loading

SugarF0x commented Jan 10, 2023 •

edited

Loading