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

fix localstorage availability check #4

Merged

Conversation

Otouto
Copy link

@Otouto Otouto commented Oct 26, 2018

Table of Contents

Description

Here is the problem if one try to run code on site with disabled access to cookie/localstorage/sessionstorage:

screen shot 2018-10-26 at 10 24 19 am

Motivation and Context

We use react-ab-test on our site and want our code running even if localstorage disabled.

How Has This Been Tested?

Run code after fix in browser with disabled access to localstorage and everything works just fine.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link
Member

@moretti moretti left a comment

Choose a reason for hiding this comment

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

Good catch!

We could probably remove the whole if block, as localStorage.setItem is already surrounded by a try...catch, so should it fall back to noopStore when localstorage is not available. 🤔

try {
let key = '__pushtell_react__';
window.localStorage.setItem(key, key);
if (window.localStorage.getItem(key) !== key) {
store = noopStore;
} else {
window.localStorage.removeItem(key);
store = window.localStorage;
}
} catch (e) {
store = noopStore;

Let's keep it for now. I'll merge it and create a new release. Thanks!

@moretti moretti merged commit d900c8a into marvelapp:master Oct 26, 2018
@moretti
Copy link
Member

moretti commented Oct 26, 2018

Released in v2.1.2

@Otouto
Copy link
Author

Otouto commented Oct 26, 2018

Agreed, if block here is unnecessary.
Thank you for the fast merge and release 💪

@Otouto Otouto deleted the fix-localstorage-availability-check branch October 26, 2018 12:42
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