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: Fixes an issue where default storage was read-only #172

Merged
merged 3 commits into from
Nov 7, 2018

Conversation

jmelberg-okta
Copy link
Contributor

@jmelberg-okta jmelberg-okta commented Nov 6, 2018

Description

Fixes an issue where checking if localStorage and/or sessionStorage was returning true, even though it cannot be written to.

See:

Example

Here is an example using iOS 10 Safari:

@@ -22,7 +22,8 @@ var storageUtil = {};
// https://connect.microsoft.com/IE/Feedback/Details/1496040
storageUtil.browserHasLocalStorage = function() {
try {
if (storageUtil.getLocalStorage()) {
var storage = storageUtil.getLocalStorage();
if (storageUtil.testStorage(storage)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: return storageUtil.testStorage(storage)

@@ -73,4 +75,15 @@ storageUtil.getCookieStorage = function() {
};
};

storageUtil.testStorage = function(storage) {
Copy link
Contributor

Choose a reason for hiding this comment

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

wonder if it's worth to check null-able of storage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that as well. Attempting to call setItem on undefined or null will throw a TypeError, which will result in returning false. I think we should be safe thanks to wrapping this in a try catch

Copy link
Contributor

@manueltanzi-okta manueltanzi-okta left a comment

Choose a reason for hiding this comment

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

🚀

@jmelberg-okta jmelberg-okta merged commit c384006 into master Nov 7, 2018
@jmelberg-okta jmelberg-okta deleted the jm-issue-117 branch November 7, 2018 00:01
jmelberg-okta added a commit that referenced this pull request Nov 12, 2018
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.

3 participants