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

Added support for sessionStorage. #2

Conversation

elliotcourant
Copy link

This is a change to support an upstream change to allow the client to
store the LD cache in the sessionStorage rather than the localStorage.

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

Describe the solution you've provided

This simply adds support for the input value for bootstrap to be either sessionStorage or localStorage rather than only localStorage.

Additional context

This is a supporting merge request for: launchdarkly/js-client-sdk#194

This is a change to support an upstream change to allow the client to
store the LD cache in the sessionStorage rather than the localStorage.
if (typeof options.bootstrap === 'string' && options.bootstrap.toUpperCase() === 'LOCALSTORAGE') {
if (typeof options.bootstrap === 'string' &&
// Support local or session storage names being passed as a bootstrap option.
['SESSIONSTORAGE', 'LOCALSTORAGE'].includes(options.bootstrap.toUpperCase())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe Array.includes() requires ES7 and I'm not sure if the transpiler will replace it with a portable method or not.

@eli-darkly
Copy link
Contributor

I know most of the implementation is in the js-client-sdk PR, I'll put general comments in this lower-level one.

I'm not totally sure whether we want to go this route. I understand the issue of multiple hashes building up in local storage, but we had been thinking about a different way to address that: simply enforcing a maximum number of those that can be stored at any given time and keeping track of how old they are, so if we exceed that maximum it just drops the oldest one. For most applications where you don't have a ton of users signing in and out on the same browser, that would provide a reasonable balance between not cluttering up the browser environment too much and still providing the desired page-loading speed gain consistently for frequent users.

Using session storage is not a bad idea, but it does mean that if I'm a user who doesn't always keep my browser open, and I log into the app at the beginning of each day, I will always see a page-load delay for feature flags on that first login. We might consider that an OK tradeoff, except that in the common case of a user who isn't sharing their local account with other people, it's not really a tradeoff that benefits them even a little since we wouldn't be filling up their local storage anyway. The decision would have to be made by the app developer and it would be a fairly arbitrary decision, since the general computer usage habits of users don't really depend on what the application is.

But we had been keeping this issue on the back burner for a while, so I appreciate the reminder. What do you think about the maximum entries/FIFO idea?

@eli-darkly
Copy link
Contributor

I forgot to add: the other thing that slightly bothers me about this, just conceptually, is that js-sdk-common is shared by 3 different SDKs, only one of which runs in a browser (unless you count the React SDK, but that's just a wrapper for the browser one). In Electron and client-side Node, there's not really any useful way to define what sessionStorage would mean, so we would have to document this as an option that is theoretically allowed in all of them but only works in one. I don't feel very strongly about this but it is a little bit counter to the way things work now.

LaunchDarklyCI pushed a commit that referenced this pull request Dec 13, 2019
initial move of code from js-client-sdk-private
@elliotcourant
Copy link
Author

I think that looking back at this that this isnt the approach to use at all. I think I do much prefer the idea of implementing something that will remove old entries in the local storage when the user changes.

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