astoilkov / use-local-storage-state

React hook that persists data in localStorage
MIT License
1.11k stars 41 forks source link

setState() doesn't always update the localStorage value #16

Closed paulmelnikow closed 3 years ago

paulmelnikow commented 3 years ago

Thanks for this useful library! I ran into a bit of a puzzle, and I'm hoping you can help. I'm using a hook created via createLocalStorageStateHook() to write to local storage, then immediately navigating to a new page to read the value from another instance of the hook.

What's happening is that, absent another render of my component, the state is not being written to local storage, so the new page unexpectedly gets empty state.

I have a reproduction which clears local storage on boot, has a button to write and navigate, and then shows whether the write was successful. Initially I had a difficult time reproducing this. Oddly, the problem only manifests when I invoke Apollo Client's useQuery() in the writer component.

While this suggests the problem could be related to Apollo Client, the ways I'm able to patch around the issue suggest that there might also be an issue in useLocalStorageState (or possibly React).

Here is the reproduction: https://codesandbox.io/s/vigorous-banzai-mhr9z?file=/src/App.js

I've found three ways to work around the problem:

  1. Patching useLocalStorageState() as follows:

            // old 
            setState((state) => {
                const isCallable = (
                    value: unknown,
                ): value is (value: T | undefined) => T | undefined => typeof value === 'function'
                const value = isCallable(newValue) ? newValue(state.value) : newValue
    
                return {
                    value,
                    isPersistent: storage.set(key, value),
                }
            })
            // new
            setState({
                value: newValue as T,
                isPersistent: storage.set(key, newValue),
            })
  2. Commenting out the call to useQuery().
  3. Wrapping the setIsReading(true) navigation transition in setImmediate(). This causes the Writer to re-render before navigating away, which fixes the problem.

So the problem seems to be something in Apollo Client preventing useLocalStorageState's asynchronous setter from firing at the time the component unmounts.

I have to wonder: does React guarantee that async dispatchers will run at the time a component is unmounting?

There is some chance of something nefarious in Apollo Client being the root cause here, however it seems more likely that either this is a bug in React, or that this is React's expected behavior, which would mean this is an interop bug in useLocalStorageState.

I think the next step here is to try to understand whether or not this is a bug in React.

astoilkov commented 3 years ago

@paulmelnikow That's a very interesting issue. I started exploring it. I will write when I have progress.

astoilkov commented 3 years ago

@paulmelnikow I understood the problem now. It's an issue in use-local-storage-state. The bug can be replicated without Apollo. I am starting to work on a solution.

P.S. If you are interested in the details I can write a comment so I can explain to you what's happening.

astoilkov commented 3 years ago

I fixed the problem but there is more work to be done. I will create a new issue that will explain the details and link to this one. You will see it when I post it here for reference.

Thanks for the time to write this detailed explanation. It's always great to see.

paulmelnikow commented 3 years ago

Yes, I am super curious to understand what you discovered. Thank you so much for the fix, too! I really like the pattern of storing relatively unimportant and low-security user-specific state in local storage, and am glad to help strengthen this thoughtful implementation. Will definitely read the issue when you post it.