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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

try/catch for localStorage calls #318

Closed
toser opened this issue Jul 31, 2019 · 6 comments
Closed

try/catch for localStorage calls #318

toser opened this issue Jul 31, 2019 · 6 comments

Comments

@toser
Copy link

toser commented Jul 31, 2019

Hey Optimizely team 馃憢

A small thing I encountered having Optimizely running in prod for some weeks now. I see a fair amount of these errors shown in our error logging service:

[OPTIMIZELY] - ERROR ${dateTime} EventProcessor: Failed to read the 'localStorage' property from 'Window': Access is denied for this document.

This is due to the fact that Google Chrome users can actively deny access to localStorage (see this https://bit.ly/2H0SMJ7).

How would the enhancement work?

This is something nobody can prevent, so I would recommend to put localStorage calls inside a try/catch block.

I don't know if these users are able to attend tests at all. If not, you could prevent them from being bucketed in the first place.

When would the enhancement be useful?

Would be nice to not see this error because no one can do anything about it.

@mikeproeng37
Copy link
Contributor

mikeproeng37 commented Jul 31, 2019

Thanks for raising this issue! Our team will look into it and get back to you as soon as we can. Can you tell us which version of the SDK you are using?

@MisterJimson
Copy link

I suggest using an in memory cache when localstorage is not useable. We do this in all our sites, common workaround.

@toser
Copy link
Author

toser commented Sep 27, 2021

@MisterJimson Thanks for the suggestion. IMHO this kind of workaround is very dependent on the use case. It might work with your own code, but third party libs could depend on the persistence of the store and break. For example with Optimizely it could break your A/B tests.
So I prefer to handle it explicitly by use case, not with a catch all workaround.

@MisterJimson
Copy link

@toser Sure, but are the A/B tests not already broken if an exception is thrown when the SDK tries to store a value?

@toser
Copy link
Author

toser commented Sep 27, 2021

@MisterJimson I don't have the insights how Optimizely works here. But in general it's better to not fake a working localstorage where it's actually just a memory store. This can cause strange behavior. Then it's better to let the browser throw an error and the lib handles the error itself.

@mikechu-optimizely
Copy link
Contributor

It looks like we later added a bit of bullet-proofing around localStorage, if I'm reading the nature of this request.

Of course, feel free to re-open this Issue with additional info.

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

No branches or pull requests

4 participants