-
Notifications
You must be signed in to change notification settings - Fork 88
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
Rewrite hasLocalStorage
check to not produce localStorage
-entries
#612
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, just some suggestions :)
src/ts/storageutils.ts
Outdated
if (StorageUtils.isLocalStorageAvailable()) { | ||
window.localStorage.setItem(key, data); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the isLocalStorageAvailable()
check does not require try-catch, that's already done internally. So we could pull it out of the try block and do an early return, or wrap the whole try-catch block with it. (same for getItem()
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, you are right. I thought that, when browser prohibits access, exceptions are triggered when actually trying to call getItem
/ setItem
, but it already happens when trying to access window.localStorage
. Hence, it's already covered in isLocalStorageAvailable
.
I removed the try-catch
es in this commit: Removed superfluous try-catch
Let's have a call if you meant it in a different way :)
src/ts/storageutils.ts
Outdated
return window.localStorage.getItem(key); | ||
} | ||
} catch (e) { | ||
console.warn(`Failed to retrieve storage entry for ${key}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO warn
is too high of a log level, as we will always check for certain localStorage keys. I'd suggest debug
or removing the log completely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the log entirely as well in Removed superfluous try-catch. Just not a fan of empty catch
blocks :)
Description
Existence of the
localStorage
API was probed so far by creating (and immediately deleting) alocalStorage
-entry. This is undesirable behaviour in case a page wishes to not have thelocalStorage
-API utilized in any way.This PR introduces the following changes:
localStorage
-API by checking ifgetItem
andsetItem
functions are available instead of setting a storage itemgetItem
/setItem
wrapper functions (e.g. if access is denied or quota got exceeded)Anyways, a
localStorage
item would still be set if certain feature of the player-UI are used. PR is a followup to introduce disabling oflocalStorage
.Checklist (for PR submitter and reviewers)
CHANGELOG
entry