imbhargav5 / rooks

Essential React custom hooks ⚓ to super charge your components!
https://rooks.vercel.app
MIT License
3.2k stars 215 forks source link

useLocalStorageState overrides local-storage item #1720

Closed sch-28 closed 1 year ago

sch-28 commented 1 year ago

Hello!

I noticed that the hook useLocalStorageState overrides local-storage items whenever the key from an already initialised useLocalStorageState is changed. Changing the key triggers the saveValueToLocalStorage function with the new key causing an override with no regard of the previous value behind that key.

Is this intentional?

sch-28 commented 1 year ago

I created a test and added an attempt to fix the issue in the pull request #1721

imbhargav5 commented 1 year ago

@sch-28 This is actually a very interesting case. Changing the key itself hmmm. So the behaviour isn't intentional but it is a strange use case. Definitely something we didn't think about.

Could you explain your particular use case again? Mainly, why the key itself needs to change? And perhaps keeping separate useLocalstorageState for a different key makes sense ?

sch-28 commented 1 year ago

@imbhargav5 So it actually came up, because the key was an id that was fetched asynchronously. And for the time it wasn't available i used a default fallback key. But after the id was fetched and the state updated itself the item stored behind the id was suddenly overwritten by the default value.. I couldn't find a way to conditionally use the hook to wait for the id to arrive :/

imbhargav5 commented 1 year ago

@sch-28 Perhaps the component containing the hook can be conditionally rendered instead? I am suggesting an alternative way to use this hook because I am concerned that dabbling with keys might open up a can of worms that are very difficult to fix and the test cases needed for this will be at another level entirely.

Would love for you to contribute to any issue in this project, but I am hesitant to merge any changes where the key itself changes in this hook.

sch-28 commented 1 year ago

Hm, while this would work i am not sure if I like this approach. I can however understand that your standpoint on this. I am not sure either; the change might create unforeseen side effects.

imbhargav5 commented 1 year ago

@sch-28 Yeah I am slightly worried about breaking the most common use case for this one while fixing the issue you pointed out. Thanks for reaching out! Closing this issue for now. Have a good day!