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

adds session storage option #247

Merged
merged 1 commit into from
Jan 4, 2016
Merged

adds session storage option #247

merged 1 commit into from
Jan 4, 2016

Conversation

MatthewVita
Copy link
Contributor

No description provided.

@booleanbetrayal
Copy link
Collaborator

@MatthewVita - rather than rename the localStorage check to hasModernStorageSupport checking both, we should have an independent check for sessionStorage.

The reason being, calling getItem on localStorage will cause exceptions if localStorage is unavailable in Safari. In your implementation, the hasModernStorageSupport check could fall-through to sessionStorage, and then during the retrieval call, we still try to call localStorage.getItem and if that returns null it'd proceed. Only ... it'd blow up at that point. Make sense?

@MatthewVita
Copy link
Contributor Author

@booleanbetrayal very good point. I'd be happy to make the change but first I'd like to explore the following:

Can we prove that it is possible for sessionStorage to be in place and localStorage to not be? Same question applies inversely, of course.

AFAIK, The Web Storage API "defines [these] two types of storage areas" [0]. Perhaps we can conclude that the spec says that both sessionStorage and localStorage are required for a correct implementation of Web Storage API, but browsers can provide overrides to disable one or the other?

[0] http://www.sitepoint.com/an-overview-of-the-web-storage-api/

@booleanbetrayal
Copy link
Collaborator

I wouldn't bother making assumptions about browser implementations @MatthewVita . I've seen too much deviance from expectation along those lines with the Storage API, particularly in iOS Safari. Better to just have the separate checks. Let me know when that lands and I'll merge it!

@MatthewVita
Copy link
Contributor Author

Good call. How that amend look?

@booleanbetrayal
Copy link
Collaborator

looks great. thanks @MatthewVita !

booleanbetrayal added a commit that referenced this pull request Jan 4, 2016
@booleanbetrayal booleanbetrayal merged commit acb0eeb into lynndylanhurley:master Jan 4, 2016
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