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

Attempt to set/get item from localstorage #205

Merged
merged 1 commit into from
Nov 12, 2016
Merged

Attempt to set/get item from localstorage #205

merged 1 commit into from
Nov 12, 2016

Conversation

IanVS
Copy link
Contributor

@IanVS IanVS commented Nov 10, 2016

Fixes #204

In Windows environments, it's possible for browsers to have a localStorage and yet not have access to it. This ends up throwing an Access is denied error when attempting to get or set from localStorage.

This change adds some additional safety checks into hasLocalStorage to set, get, and remove a test item inside the try block.

@rt2zz
Copy link
Owner

rt2zz commented Nov 10, 2016

interesting. Can we simplify the check to simply running a localStorage.getItem('test key') call? I would assume that would throw and there is no need to also test set and remove?

@rt2zz
Copy link
Owner

rt2zz commented Nov 10, 2016

also does sessionStorage suffer from the same quirk?

@IanVS
Copy link
Contributor Author

IanVS commented Nov 10, 2016

Can we simplify the check to simply running a localStorage.getItem('test key')

That would handle this particular case, yes. But, it does not save us if localStorage is at capacity and cannot be written to, which is where the setItem is useful.

does sessionStorage suffer from the same quirk?

Honestly I don't know. I don't think I've been seeing any sessionStorage errors, so I wouldn't know how to test it. I'd say leave it alone unless it's determined to be a problem.

@IanVS
Copy link
Contributor Author

IanVS commented Nov 10, 2016

This is a pretty good thread with different options for checking localStorage: https://mathiasbynens.be/notes/localstorage-pattern#comment-9

@rt2zz rt2zz merged commit 89ee329 into rt2zz:master Nov 12, 2016
@rt2zz
Copy link
Owner

rt2zz commented Nov 12, 2016

👍 I decided to simplify the check to just getItem for now. This module is very sensitive to startup time and I want to be conservative with anything that could impact this.

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