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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add persist middleware #229

Merged
merged 4 commits into from
Nov 8, 2020

Conversation

AnatoleLucet
Copy link
Collaborator

@AnatoleLucet AnatoleLucet commented Nov 4, 2020

Hi 馃憢 I saw this comment so I've decided to open a pr for the middleware I've made and shared here.

I've been using it for multiples months on a project at work, and it actually works nicely given its simplicity.
But I still think there's some quite annoying drawbacks that we could probably improve:

  • The obligation to give a name to the middleware. It'd be preferable to not hard code the name so the middleware could generate unique names/ids for the user. But I can't come up with a way to do this.
  • Rehydration & merge & update of a stored state that has changed since. I don't think this is possible (?), but it still'd be nice to erase the old (incompatible) stored state by the new one (so there would be some data loss). I have some ideas on how to choose if the stored state needs to be erased or not, but I'd like to have the maintainers' opinions first.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 4, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit cced1a6:

Sandbox Source
React Configuration
React Typescript Configuration

@dai-shi
Copy link
Member

dai-shi commented Nov 4, 2020

Thanks for opening up this. I'm happy to discuss and I also hope someone to chime in.

First things first, I would like to factor out localStorage and make it usable with AsyncStorage or any other storage implementations. How does that sound?

@AnatoleLucet
Copy link
Collaborator Author

AnatoleLucet commented Nov 4, 2020

For sure it sounds great. I've already responded to someone asking how he could port this to AsyncStorage.
I might need to give a glance at the hook first to understand how it works and how this could be done. But maybe you have something in mind?

@dai-shi
Copy link
Member

dai-shi commented Nov 4, 2020

Let me see, it shouldn't have to be a hook.

export const persist = (
  config,
  name,
  storage,
) => (set, get, api) => {
  const store = config(
    (payload) => {
      set(payload) // payload can be a function
      storage.setItem(name, JSON.stringify(get())
    },
    get,
    api
  )

  (async () => {
    const storedState = JSON.parse(await storage.getItem(name)) // needs error handling
    api.setState(storedState);
  })()

  return store
}

@AnatoleLucet
Copy link
Collaborator Author

That's nice. How should the error handling be done? By logging a message with the Error object?

@dai-shi
Copy link
Member

dai-shi commented Nov 4, 2020

Hm, don't know. It would be logged anyway. We need to think about the use case how we would recover from errors.

@AnatoleLucet
Copy link
Collaborator Author

Well, if an error is thrown during the parsing or the "getting" (getItem), it'll be caught (by our try/catch) before setting the state. So it shouldn't crash the user's app nor the store.

@dai-shi
Copy link
Member

dai-shi commented Nov 4, 2020

Are you sure? Not sure what is meant by "our try/catch".

Anyway, it can be an edge case. We can start trying our basic idea. How do we proceed? Does my code snippet really work? Would you try? Would you incorporate it in this PR?

@AnatoleLucet
Copy link
Collaborator Author

Are you sure? Not sure what is meant by "our try/catch".

  (async () => {
    try {
      const storedState = JSON.parse(await storage.getItem(name))
      api.setState(storedState);
    } catch (e) {
      // catches JSON.parse and storage.getItem errors
    }
  })()

image
image

Anyway, it can be an edge case. We can start trying our basic idea. How do we proceed? Does my code snippet really work? Would you try? Would you incorporate it in this PR?

I'll try it and if it does, I'll refactor the pr.

@AnatoleLucet
Copy link
Collaborator Author

How do you build the package so it can be linked? yarn prebuild && yarn build && yarn postbuild?

@dai-shi
Copy link
Member

dai-shi commented Nov 4, 2020

Just yarn build is fine.
Also, if you push the change into this PR, codesandbox ci will build it for you.

@AnatoleLucet AnatoleLucet force-pushed the feat/persit-middleware branch 2 times, most recently from da8395a to 1e35404 Compare November 5, 2020 10:22
@AnatoleLucet
Copy link
Collaborator Author

@dai-shi it seems to work https://codesandbox.io/s/react-typescript-forked-rj85m?file=/src/App.tsx (you can try by commenting / uncommenting the useEffect).

@dai-shi
Copy link
Member

dai-shi commented Nov 5, 2020

Confirmed. 馃憤

src/vanilla.ts Outdated Show resolved Hide resolved
src/middleware.ts Outdated Show resolved Hide resolved
src/middleware.ts Outdated Show resolved Hide resolved
@hmans
Copy link

hmans commented Nov 5, 2020

I don't know if this would bust the scope of this change/feature wide open (I do suspect that it does, though), but how do you feel about expressing changes as JSON Patches (considering that atomic updates to a zustand store may affect more than 1 key)?

Having written this, I'm pretty sure that this would go beyond what you're trying to do here. I just wanted to point out that JSON Patch et al are a thing, and it would be great to have persistence glue based on it for zustand stores (it would allow some fun implementations of differential synchronisation.) If there is some interest in this stuff, I'd be happy to whip something up, as my current project is sort of heading into its general direction.

@dai-shi
Copy link
Member

dai-shi commented Nov 5, 2020

@hmans differential synchronization sounds interesting. we can try that road if someone is interested, but it can be another middleware as you implied.

This reminds me of something. How about factoring out JSON.parse/stringify and make them customizable? It should be easy if they are synchronous functions or even async doesn't matter. What do you think? @AnatoleLucet

@AnatoleLucet
Copy link
Collaborator Author

AnatoleLucet commented Nov 5, 2020

@dai-shi sure, why not. Do you think of a custom callback given through the arguments (then we might need to create an option object as the third argument of persist) like what we're doing with the storage? And only the parse/stringify or a whole set/get function?

@dai-shi
Copy link
Member

dai-shi commented Nov 5, 2020

Maybe parse/stringify only. We don't need to cover 100% use cases.
I might call it serialize/deserialize, but whichever is ok.
option object sounds good. I was thinking about two additional arguments, which can be too many.

@AnatoleLucet
Copy link
Collaborator Author

There you go.

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good.

Feel like we should test it with RN. Any volunteers?

src/middleware.ts Outdated Show resolved Hide resolved
src/middleware.ts Outdated Show resolved Hide resolved
@dai-shi
Copy link
Member

dai-shi commented Nov 6, 2020

Should we have a small description in readme.md?

@AnatoleLucet
Copy link
Collaborator Author

@dai-shi
Copy link
Member

dai-shi commented Nov 6, 2020

And only the parse/stringify or a whole set/get function?

Now, after looking at the readme, I understand what this really means.
If we customize the whole set/get, we don't need name and storage options, right?
But, maybe it's too much customization, and the current options are fine, I think.
Just want to make it sure if it's aligned with your thought.

@AnatoleLucet
Copy link
Collaborator Author

I was just thinking about letting the user do this whole async function :

  ;(async () => {
    try {
      const storedState = await storage.getItem(name)
      if (storedState) set(await deserialize(storedState))
    } catch (e) {
      console.error(new Error(`Unable to get to stored state in "${name}"`))
    }
  })()

But you're right, this is probably too much customization.

@dai-shi
Copy link
Member

dai-shi commented Nov 6, 2020

OK, we are on the same page. I'll think about again if we have a better option. I think our primary target is localStorage in web and AsyncStorage in RN.

Meanwhile, would you check #229 (comment) again? It's not resolved.

@AnatoleLucet
Copy link
Collaborator Author

Meanwhile, would you check #229 (comment) again? It's not resolved.

Done

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

src/middleware.ts Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@dai-shi
Copy link
Member

dai-shi commented Nov 8, 2020

I don't know why codesandbox ci doesn't finish until now, but let me merge it since all other tests are passing.

@dai-shi dai-shi merged commit 055a3e9 into pmndrs:master Nov 8, 2020
@calendee
Copy link

calendee commented Nov 8, 2020

Thanks so much for adding this functionality! I had created my own wrapper for doing this in React Native & React Native Web. However, this is much more elegant!

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

Successfully merging this pull request may close these issues.

None yet

4 participants