astoilkov / use-local-storage-state

React hook that persists data in localStorage
MIT License
1.09k stars 39 forks source link

Debouncing updates produces weird behaviour #31

Closed h3rmanj closed 2 years ago

h3rmanj commented 2 years ago

Using the hook returned from createLocalStorageStateHook produces weird behavior when debouncing updates.

Using a controlled input, with debouncing using a useEffect, the updates eventually propagates to all components (not always all) using the state.

Using an uncontrolled input, with normal debouncing, the updates seemingly propagates randomly to the components using the state.

LocalStorage is always updated.

We've simplified our case and created a reproducible example in this codesandbox:
https://codesandbox.io/s/use-local-storage-state-debounce-u3p7m?file=/src/App.tsx

I've quickly gone through the source for use-local-storage-state, and can't find anything to indicate this should happen. I'm wondering if we've hit some limitation of React or something.

h3rmanj commented 2 years ago

I figured it out! I remember reading about how react batches setState only in events and effects in React 17. See https://github.com/reactwg/react-18/discussions/21

So I thought about what happens when we call the setState function from the use-local-storage-state hook; it calls the setState for everyone using the hook.

And since we're calling this from a timeout, all these setState calls won't be batched. I'm not exactly sure why React ends up skipping updates when using timeouts from an uncontrolled input though.

React 18 automatically fixes this with the new createRoot API: https://codesandbox.io/s/use-local-storage-state-debounce-react-18-csi5b?file=/src/index.tsx

In React 17, we can force batching with unstable_batchedUpdates: https://codesandbox.io/s/use-local-storage-state-debounce-batched-updates-k2b9v?file=/src/ControlledInput.tsx:331-369

So you could technically fix this in this library by using unstable_batchedUpdates. But seeing as forcing batched updates is an undocumented unstable API, and this will be automatically fixed with React 18, and also this is an uncommon edge case, I don't think it's necessary.

Feel free to reopen should you want to support this in React 17 though.

astoilkov commented 2 years ago

Thanks for reporting this and all the detailed information. I think I should fix this. It's both slow and buggy for React 17 which I believe will be popular for some more time. At some point, I will remove it.

h3rmanj commented 2 years ago

Do note that unstable_batchedUpdates comes from react-dom, so it'll need to be added as a peerDependency as well.

astoilkov commented 2 years ago

Hmm, didn't think about that. Thanks.

astoilkov commented 2 years ago

I fixed the issue and released an update. Thanks for the time to report and investigate the issue!