fix: fixed up bug(#549) -Init use useState don't work in useLocalStorage

[LonelyFellas]

#549

[changeset-bot]

🦋 Changeset detected

Latest commit: 0f286bb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
usehooks-ts	Patch
www	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[jraoult]

Thanks, @LonelyFellas, for trying to tackle that. That said, my gut feeling would be that the fix has to do with changing the useLocalStorage.ts file:

usehooks-ts/packages/usehooks-ts/src/useLocalStorage/useLocalStorage.ts

Line 122 in 0f286bb

const setValue: Dispatch<SetStateAction<T>> = useEventCallback(value => {

to

const setValue: Dispatch<SetStateAction<T>> = useCallback(value => {

with the appropriate dependency list, rather than changing how useEventCallback behaves.

[LonelyFellas]

@jraoult Firstly, using useCallback in useLocalStorage is a good choice and it also works. However, regarding this bug, it's not just an issue with useLocalStorage; it's also a problem with useEventCallback. Therefore, I still think it's better to modify useEventCallback. How do you think?

[jraoult]

@LonelyFellas, just a disclaimer: I'm not a repo maintainer, so my opinion is not super relevant here.

My first point is that useCallback is the proper hook in this context instead ofuseEventCallback. My understanding is that useEventCallback stems from (or precedes) the now abandoned useEvent RFC. It is explicitly built to stabilise the reference to a callback that is never run during a render phase. If a callback can/needs to be run in the render phase and we still want to stabilise the reference, we need to use useCallback instead (cf. When useEvent should not be used?).

My second point would be that your proposed patch introduces a change in the contract of useEventCallback. Now useEventCallback return callback can be used in the render phase, which it shouldn't by design, potentially breaking the code using it due to the lack of dependency tracking. Here's a quote from the RFC:

This makes it safe to preserve their identity despite the changing props/state inside. Because they can't be called during rendering, they can't affect the rendering output — and so they don't need to change when their inputs change (i.e. they're not "reactive").

But again, it would be up to @juliencrn to decide if my initial issue is something they think makes sense and then decide the proper way to implement it.

[juliencrn]

Thanks for the proposition, but we'll abandon this change. (#549 (comment)).

[juliencrn]

@all-contributors please add @jraoult for bug and review

[allcontributors]

@juliencrn

I've put up a pull request to add @jraoult! 🎉

[juliencrn]

@all-contributors please add @LonelyFellas for code

[allcontributors]

@juliencrn

I've put up a pull request to add @LonelyFellas! 🎉