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

♻️ ability to specify optional provider for sessionStorage #14

Closed
wants to merge 2 commits into from

Conversation

Rendez
Copy link

@Rendez Rendez commented Nov 11, 2020

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:

  • useLocalStorageState
  • useSessionStorageState
  • createLocalStorageStateHook
  • createSessionStorageStateHook

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

  • useStorageState / usePersistedState
  • createStorageStateHook / createPersistedStateHook

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
Copy link
Author

Rendez commented Nov 12, 2020

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
Copy link
Owner

Hi @Rendez,

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

  • Does your last comment mean you don't need sessionStorage any longer?
  • Did you go through the issue where this has been discussed – Session Storage support? #9?

@Rendez
Copy link
Author

Rendez commented Nov 16, 2020

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: #9 (comment). 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
Copy link
Owner

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
Copy link
Owner

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:

  • as your pull requests shows I need to make a big breaking change in order to add support for sessionStorage
  • the size of the module will increase without any benefit for most users
  • the complexity of the module will increase
  • I believe in modules which are one purpose only, I am mainly inspired by @sindresorhus type of modules
  • use-local-storage-state name isn't correct when using it for sessionStorage

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
Copy link
Author

Rendez commented Nov 23, 2020

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
Copy link
Owner

What problem do you see with duplicate tests?

@astoilkov
Copy link
Owner

I am closing this because:

  • we didn't continue the conversation
  • I haven't seen much interest for sessionStorage support from other users
  • this pull request can no longer be merged because of the big changes in the library architecture

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

@astoilkov astoilkov closed this Feb 25, 2021
@Rendez
Copy link
Author

Rendez commented Feb 26, 2021

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

@astoilkov
Copy link
Owner

❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants