astoilkov / use-local-storage-state

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

♻️ ability to specify optional provider for sessionStorage #14

Closed Rendez closed 3 years ago

Rendez commented 4 years ago

I need this library to work with sessionStorage as well and I find the implementation really neat. As you'll notice, I haven't changed the exported functions's name to reflect the changes yet, as I would wait for your feedback first, but one option is to export 4 functions:

The breaking API alternative is to provide 2 functions instead, like now, but rename them to these (or similar):

Also, though test coverage remains at 100%, if you'd like to avoid the duplication created, I could improve on that too.

Looking forward to your feedback.

Rendez commented 4 years ago

I became painfully aware that sessionStorage does not emit an storage event because the Storage is isolated between tabs/windows. It will emit though for iframes. In my particular use case, localStorage will be needed to sync. tabs' state - I now understand why there wasn't support until now for sessionStorage.

I believe using sessionStorage as a provider is a very valuable option for many other use cases, where sync isn't important, or sync happens between frames. Often when using localStorage, one has to clean up after himself, so it's a tradeoff I think users will be happy to decide upon.

astoilkov commented 4 years ago

Hi @Rendez,

Thank you very much for your contribution. I hope you don't mind a few questions:

Rendez commented 3 years ago

Ok, I didn't go through that issue initiall, but now I have...

I still need sessionStorage, but it suffers from the same shortcoming as explained in: https://github.com/astoilkov/use-local-storage-state/issues/9#issuecomment-668072283. That's why we'll be using localStorage instead, because sessionStorage's event doesn't behave the same way (only works within iframes in the same tab).

The issue with localStorage in my opinion is that is too persistent, it sticks around forever and it's hard to "mend" it once you've shipped it to production. I have implemented our feature the next way:

{
  [id]: ExpirationTimestamp,
  [id2]: ExpirationTimestamp,
}

If a flag is false, it's removed from the map, if it's added it's added with a timestamp value (+2 days). Once we "set" one more value using the hooks' state setter, we go over each other time and check it against TTL. Furthermore, if all are expired and we're setting it to "false", we remove the entire entry. This is done inside useEffect() using .clear().

This is a lot of bookeeping just because we are forced to use localStorage. Which is fine. However, not everyone needs tab-synchonization, or don't care about that feature as much, because they simply want to keep state upon (refreshes of the page). sessionStorage is a non-so-serious approach, offers some benefits and allows you to be more lax about keeping your Storage keys around, since you know they'll be wiped-out eventually when the user closes the browser.

I am with you however, in trying to find meaningful use cases that justify adding sessionStorage to this package. I understand it quite well.

astoilkov commented 3 years ago

I am currently working on some Big Sur issues with our app and am busy. I will answer you at the first opportunity. There isn't a chance I will forget.

astoilkov commented 3 years ago

Thanks for the detailed explanation.

I have another idea for solving the support of sessionStorage. I am thinking of creating a module called use-session-storage-state. These are the reasons behind my thinking:

Making another module which is very close to this one solves all of the above issues.

What do you think? Do I make sense?

Rendez commented 3 years ago

I totally like your reasoning. The only thing I would keep in mind is to make sure that tests aren't duplicated. Maybe that would require this to become a monorepo so we can write shared tests?

astoilkov commented 3 years ago

What problem do you see with duplicate tests?

astoilkov commented 3 years ago

I am closing this because:

I am sorry that your contribution didn't get merged. I am sad when this happens.

Rendez commented 3 years ago

@astoilkov in retrospective, I learnt a lot and there is nothing to be sorry about, you were very open about my idea :)

astoilkov commented 3 years ago

❤️