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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Different behaviour of LocalStorage and AsyncStorage #583

Closed
steida opened this issue Oct 7, 2021 · 35 comments
Closed

Different behaviour of LocalStorage and AsyncStorage #583

steida opened this issue Oct 7, 2021 · 35 comments
Labels
middleware/persist This issue is about the persist middleware

Comments

@steida
Copy link

steida commented Oct 7, 2021

In my app, I'm using @react-native-async-storage/async-storage because I plan to support React Native.
But because React Native is not expected to be supported soon, I decided to switch back to LocalStorage
and my app stopped working. If I will add setTimeout to state.dispatchStoreRehydrated();, it works.
It's probably because LocalStorage is sync while AsyncStorage isn't.

onRehydrateStorage: () => (state) => {
  if (state)
    // Fix for LocalStorage. With AsyncStorage, setTimeout
    // is not required.
    setTimeout(() => {
      state.dispatchStoreRehydrated();
    }, 0);
},
@dai-shi dai-shi added the middleware/persist This issue is about the persist middleware label Oct 7, 2021
@AnatoleLucet
Copy link
Collaborator

Not sure what you mean by "my app stopped working". Can you send enough code to reproduce (the full code of your zustand store, a component that doesn't work as expected, etc.)?

Though, there's indeed a difference between a sync and an async storage. With a sync storage, your zustand store will be hydrated at the initial render. With an async storage, your zustand store will be hydrated asynchronously (a.k.a. at some point after the initial render).

@steida
Copy link
Author

steida commented Oct 8, 2021

Sorry, I thought that little example will be enough to demonstrate that LocalStorage shall be Async by default to ensure the same behavior with AsyncStorage. I will make a failing example.

@AnatoleLucet
Copy link
Collaborator

Conditional async/sync hydration is intended. Always making the hydration async was causing more trouble and frustration than conditionally making it async.

People using synchronous storage needed to use hacks like this to know if their store had been hydrated already.

See #346

@steida
Copy link
Author

steida commented Oct 8, 2021

I will try to explain why the behavior of onRehydrateStorage should be consistent. Imagine someone is writing an app for the web with the default LocalStorage. Later they realize LocalStorage is not enough and IndexedDB (or another async storage) should be used instead. Suddenly, the app is rendered twice instead of once. Initial render with the initial state, then the second render with the state from storage. As I think about it, it can cause a server/client HTML mismatch when some HTML is rendered on the server-side with the initial store state and on the client with the stored (not initial) state.

@steida
Copy link
Author

steida commented Oct 8, 2021

People using synchronous storage needed to use hacks like this to know if their store had been hydrated already.

I saw that and I had that problem too. I think Zustand API is trying to be too smart which leads to inconsistent behavior. The initial state shall always be the initial, and only then updated with the stored state. That's what AsyncStorage forces Zustand to do. The storage should not change how Zustand behaves I suppose.

I can't imagine a use case where someone would prefer sync rehydration skipping the initial state because that must complicate server-side rendering by default.

@steida
Copy link
Author

steida commented Oct 8, 2021

I think such inconsistent API is confusing. Zustand should not fight with async storage API but embrace it. All storages are async, Sync LocalStorage is a very rare exception and definitely not recommended because it's blocking the main thread. Propagating that storage has been hydrated is not a hack but a useful piece of information.

I can help you to redesign it for version 4 if you want.

@AnatoleLucet
Copy link
Collaborator

Don't get me wrong, I totally agree with all that. And that's why I was a bit skeptical at first.
But on the other end, most people use this middleware to save user settings, auth tokens, or things like such... which all are stuff that you want right away in a synchronous way.

I don't think we can satisfy both wishes (but if you do, please share 😄).
So either way, someone's always going to end up with a hack to make it work the way he thinks it should.

@steida
Copy link
Author

steida commented Oct 8, 2021

@AnatoleLucet Speaking of "stuff that you want right away in a synchronous way.", not having the ability to know when the store has been flushed is complicating things for me as well. I store auth info before full redirect (to force iOS Safari to show save password dialog) and I can't be sure data were successfully flushed (no API in Zustand for that) so my app has to use artificial setTimeout which is a code smell.

@AnatoleLucet
Copy link
Collaborator

I think such inconsistent API is confusing. Zustand should not fight with async storage API but embrace it. All storages are async, Sync LocalStorage is a very rare exception and definitely not recommended because it's blocking the main thread. Propagating that storage has been hydrated is not a hack but a useful piece of information.

I can help you to redesign it for version 4 if you want.

I mean, if you can answer: how can sync storage users not pay to cost of async storages & consistency (which they don't use... because... sync), then sure, let's do it.

@steida
Copy link
Author

steida commented Oct 8, 2021

I mean, if you can answer: how can sync storage users not pay to cost of async storages & consistency (which they don't use... because... sync), then sure, let's do it.

Which costs exactly? They can use it as they are already using it. The only problem I can imagine is saving something then unloading tab immediately without waiting for success. Or did you mean something else?

@AnatoleLucet
Copy link
Collaborator

@AnatoleLucet Speaking of "stuff that you want right away in a synchronous way.", not having the ability to know when the store has been flushed is complicating things for me as well. I store auth info before full redirect (to force iOS Safari to show save password dialog) and I can't be sure data were successfully flushed (no API in Zustand for that) so my app has to use artificial setTimeout which is a code smell.

Sorry if I misunderstand here, but isn't that something that could be solved by #267?

@AnatoleLucet
Copy link
Collaborator

AnatoleLucet commented Oct 8, 2021

I mean, if you can answer: how can sync storage users not pay to cost of async storages & consistency (which they don't use... because... sync), then sure, let's do it.

Which costs exactly? They can use it as they are already using it. The only problem I can imagine is saving something then unloading tab immediately without waiting for success. Or did you mean something else?

If we make the hydration consistently async, people'll need hacks to know when they can use the store's data (even if they use sync storages, which is kind of the goal of using a sync storage (so they do pay a cost)).

If we keep the current, conditional, behavior, people (you in this case) will need hacks to make it consistently async (as you demonstrated with your setTimeout example).

See where I'm going? Whatever we go for, someone's always going to need a hack to make it work the way they want.
And if you have something in mind to solve that, then I'm down to make the change.

@steida
Copy link
Author

steida commented Oct 8, 2021

Yes, probably with something like persist.onComplete(callback), which would be called immediatelly when there is no pending saving or after saving.

And global isPending of course.

Two use cases:

  1. Do location redirect after persist was flushed.

  2. Don't allow tab closing via beforeunload until persist was flushed.

@AnatoleLucet
Copy link
Collaborator

AnatoleLucet commented Oct 8, 2021

That's not fixing the hack, that's just embedding it.
Sync users still pay the cost of not having the data at the initial render. And yes, that's almost solved by React batching renders (though I'm not sure if they batch the initial render since it might increase TTI 🤔).
But what if you have 2 or more individual stores using this middleware. Then you'd need to programmatically wait for each of them? That's adding code in your codebase that shouldn't be needed, because again, that's the goal of synchronous storages. I mean, that's still increasing the cost of consistency...

@steida
Copy link
Author

steida commented Oct 8, 2021

If we make the hydration consistently async, people'll need hacks to know when they can use the store's data

They will always have to hack something. With "no hacking because it's sync" approach, they will have to hack SSR with Next.js which is pretty common. Current sync approach is "hacks free" only for SPA (app without any server side rendering).

If someone needs to know when data were loaded, it's exactly a usecase to always keep initial state. For example a settings. Initial state is null, because nothing has been set yet. The app is forced to postpone rendering until storage was hydrated, which is a good think, because some apps can decide to wait while the other can render partially, for SEO or UX reasons.

I think it's fair to allow Zustand users to choose strategy without setTimeout hacks. Explicit > Smart.

@AnatoleLucet
Copy link
Collaborator

They will always have to hack something.

Why even wanting to make the change to full async then? Is it just to transfer the hack somewhere else?

I think it's fair to allow Zustand users to choose strategy without setTimeout hacks. Explicit > Smart.

That's true though. It'd add even more verbosity to the middleware (which is already a bit bloated in my opinion), but that's the only common ground I can see here.

@steida
Copy link
Author

steida commented Oct 8, 2021

I will think about it as well. Need to take a look at Zustand source code in detail.

Meanwhile, please point me to latest maintainers ideas to help me to understand where Zustand is going.

Thank you a lot!

@steida
Copy link
Author

steida commented Oct 8, 2021

As for more stores, I have to admit I don't understand why it should be useful, it's for separate bundles?

@AnatoleLucet
Copy link
Collaborator

AnatoleLucet commented Oct 8, 2021

I will think about it as well. Need to take a look at Zustand source code in detail.

I might be wrong about "it'd add even more verbosity to the middleware" though.
Maybe conditionally (in "dumb" mode) overriding the storage with something like this might do the job:

const promisify = <T extends Function>(fn: T) => (...args: Parameters<T>): Promise<ReturnType<T>> =>
  new Promise((resolve) => resolve(fn(...args)))

const { setItem, getItem } = getStorage()

storage.getItem = promisify(getItem)
storage.setItem = promisify(setItem)

https://github.com/pmndrs/zustand/blob/main/src/middleware.ts#L333

Meanwhile, please point me to latest maintainers ideas to help me to understand where Zustand is going.

Sure, issues might be the best place 😄
middleware/persist This issue is about the persist middleware

@AnatoleLucet
Copy link
Collaborator

AnatoleLucet commented Oct 8, 2021

As for more stores, I have to admit I don't understand why it should be useful, it's for separate bundles?

I personally like to create a bunch of stores, each with its own context. A bit like redux reducers for instance.
So you might have one for auth, another for settings, etc.

@steida
Copy link
Author

steida commented Oct 8, 2021

I personally like to create a bunch of stores, each with its own context. A bit like redux reducers for instance.
So you might have one for auth, another for settings, etc.

Isn't that what slices are for? Global state is one, especially when persisted I suppose.

@AnatoleLucet
Copy link
Collaborator

I personally like to create a bunch of stores, each with its own context. A bit like redux reducers for instance.
So you might have one for auth, another for settings, etc.

Isn't that what slices are for? Global state is one, especially when persisted I suppose.

It can be. Just depends on how you like to structure your stores.

@steida
Copy link
Author

steida commented Oct 8, 2021

As I see it, the current persist implementation should be reimplemented.

  1. LocalStorage can't stay sync because of all other async storages.
  2. Because persist is async, it has to expose isPending and some callback for persist completed.

That would cover all my and other people use-cases I suppose.
Can I help you with that somehow?

@AnatoleLucet
Copy link
Collaborator

I might have misunderstood. I was just thinking of adding an option to toggle between smart and dump mode.

@steida
Copy link
Author

steida commented Oct 8, 2021

Do you mean sync and async mode for LocalStorage?

@AnatoleLucet
Copy link
Collaborator

AnatoleLucet commented Oct 8, 2021

Yes. Basically "promisifying" the given storage.

Though I'm not even sure something like this should belong in the middleware.
Because you can already do something like this:

const promisify = (fn) => (...args) => new Promise((resolve) => resolve(fn(...args)))

const asyncLocalStorage = {
  getItem: promisify(localStorage.getItem),
  settItem: promisify(localStorage.setItem),
}

export const useStore = create(persist(
  (set, get) => ({
    ...
  }),
  {
    name: "my-store",
    getStorage: () => asyncLocalStorage ,
  }
))

which in my opinion is definitely not a hack.

@steida
Copy link
Author

steida commented Oct 8, 2021

Can you explain to me why smart/sync mode is required, please?
As I see it, it only complicates SSR and I don't see any benefit of it.
It's because with async mode people can accidentally set a new state before rehydrating?
What am I overlooking?

@AnatoleLucet
Copy link
Collaborator

AnatoleLucet commented Oct 8, 2021

Can you explain to me why smart/sync mode is required, please?

As I said, "most people use this middleware to save user settings, auth tokens, or things like such... which all are stuff that you want right away in a synchronous way".

Fun fact: before we made the hydration a bit smarter about async/sync (so back when it was always async), a coworker using the middleware on a project at work actually asked me why the storage's data wasn't in the zustand store at the initial render since he was using a sync storage. When I explained to him why, he found it a bit annoying that he had to add a bit of logic to wait for hydration even if he was using a sync storage.
Conclusion:

  • people using a sync storages expect the hydration to be done synchronously
  • people using an async storages understand why the hydration is done asynchronously and so are fine to deal with its cons

As I see it, it only complicates SSR and I don't see any benefit of it.

SSR is more and more common, but not everyone uses it. But even with that aside, the fact that "async hydration is easier to manage with SSR" (which I don't think is true, it just depends on how you implement it) is just abusing the fact that the hydration goes away in a promise and doesn't interfere with your SSRender (which is something that can be programmatically done with sync hydration using a condition to check if you're on the server or not).

It's because with async mode people can accidentally set a new state before rehydrating?

I guess people should be aware of that (or at least rapidly figure out why) if they use an async storage?

What am I overlooking?

I think your vision is kind of biased by focusing on SSR and your personal usage of zustand and this middleware.
It's just a persist middleware, but there's just so much use cases to think about 😄

The example I gave you above of manually making a sync storage async is the best solution I can think of for what you want.
I believe this is a perfectly valid option, and the current behavior of the middleware shouldn't be changed. But if you're not agreeing with that, I'm still up for debate.

@steida
Copy link
Author

steida commented Oct 8, 2021

Thank you for your thorough explanation and the solution you provided. I am closing this issue for now. Maybe I will create a new one with a more specific use case.

As I see it, ideally there should be sync persist and async persist because it's two different use cases.

@steida steida closed this as completed Oct 8, 2021
@steida
Copy link
Author

steida commented Oct 8, 2021

Sorry for reopening, but it probably belongs here anyway.

I believe both use-cases (sync and async) are valid, but common API for that does not make sense.

What I am suggesting is to split implementation to syncPersist (LocalStorage or SessionStorage or even cookie) and asyncPersist (for everything else). It can be named persist/asyncPersist.

It would simplify implementation and make it explicit as well.

What do you think?

@steida steida reopened this Oct 8, 2021
@AnatoleLucet
Copy link
Collaborator

How is that different from an option to toggle sync hydration?

It would, indeed, be more explicit, but I believe it will lead to even more confusion around what the users should use.
Because if I understand your proposal correctly, it's not as straightforward as using syncPersist for a sync storage and asyncPersist for an async one. You use one or the other depending on how you want the hydration to be done (right?), which I believe is not something most zustand users should worry about.

Let's take this from a new angle.
If you were trying to get something from a sync storage without using zustand:
Would you prefer the synchronous way?

const data = useMemo(() => localStorage.getItem("data"), [])

Or the asynchronous way?

const [data, setData] = useState(null)

useEffect(() => {
  setData(localStorage.getItem("data"))
}, [])

By what you're saying from the start of this conversation, I believe you'd prefer the asynchronous way. But by what I've seen since we've created this middleware, this is not what most people want when they're using a synchronous storage.

If you're worrying about "explicity", I found it really straightforward:

  • sync storage -> sync hydration
  • async storage -> async hydration

But it might just be me because I know the inner workings of this middleware and how it evolved. And if that's the case, it can easily be solved with a bit of documentation around async storages and what is means for hydration.

@steida
Copy link
Author

steida commented Oct 10, 2021

I would prefer a synchronous way for sync storage and an asynchronous way for async storage, hence two different APIs or just one but async. Yes, documentation always helps. Maybe it's problem on my side and I should create my own persist. On the other side, I am not doing anything special, I was just using async storage because I plain React Native, but because I postponed it, I switched back to LocalStorage and had to add artificial setTimeout to have the same behavior.

Another issue is that async persist shall expose pending saving and callback for onComplete, otherwise Zustand users can accidentally lose data when they close the tab too soon.

@AnatoleLucet
Copy link
Collaborator

Since the middleware now have an api that can easily be used to check for hydration, some docs about async hydration, and a few examples to demonstrate how to reactively check hydration status, I believe this issue can be closed.
@steida Feel free to give any feedback.

Maybe it's problem on my side and I should create my own persist

I guess you could create your own wrapper middleware around persist to promisify the storage.
Something like this maybe ?

export const asyncPersist = (config, options) => {
  const { setItem, getItem, removeItem } = options.storage;

  options.storage = {
    setItem: async (...args) => setItem(...args),
    getItem: async (..args) => getItem(...args),
    removeItem: async (...args) => removeItem(...args),
  };

  return persist(config, options);
};


const useStore = create(asyncPersist(
  (set, get) => ({
    ...
  }),
  {
    ...
  }
))

@steida
Copy link
Author

steida commented Nov 22, 2021

LGTM

@a-toms
Copy link

a-toms commented Sep 14, 2022

This is not working currently. See #1145 for a fix as of writing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
middleware/persist This issue is about the persist middleware
Projects
None yet
Development

No branches or pull requests

4 participants