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

Fix useLocalStorage API #257

Closed
wants to merge 1 commit into from

Conversation

pedrobslisboa
Copy link

The way useLocalStorare was built does not reflect useState behavior.

You guys are passing the getter to the setter instead of getting the previous value, which does not reflect the useState API. It can produce a lot of bugs since the useState api works async and need to get the previous value on the setter, instead of directly the getter, to work correctly.

You can check out the problem running it here: https://codesandbox.io/s/uselocalstorage-sample-mistake-uxms2b

You can check out the solution working here: https://codesandbox.io/s/uselocalstorage-sample-working-right-4ls3ox?file=/src/App.tsx

Same issue:
uidotdev/usehooks#156

@pedrobslisboa
Copy link
Author

@juliencrn

@henriqemalheiros
Copy link

Ran into the same issue. There is an older PR trying to solve it too (#242). Yours is missing the window.dispatchEvent that notifies other useLocalStorages that have the same key.

@valyrie97
Copy link

Ill piggy back on here:

  • There's two invocations to value(prev) which doesnt comply with useState.
  • window being undefined is now defined to silently "fail"
  • Could you write a test to demonstrate the problem and the fix? (I'm not sure if its the same as in useLocalStorage complies with useState #242 but there is a test case there if you want to reference)

@juliencrn
Copy link
Owner

Fixed by #436, thanks all

@juliencrn juliencrn closed this Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants