Skip to content
This repository has been archived by the owner on Feb 17, 2021. It is now read-only.

Fix sessionStorage exceeds quota error in Safari #89

Merged
merged 4 commits into from
Jun 5, 2014

Conversation

timlindvall
Copy link
Collaborator

Fixes issue #86. We're adding an attempt to write to sessionStorage before we set the hasSessionStorage flag. This catches scenarios where sessionStorage is available but cannot be written to.

I'm also contemplating adding another try/catch block in setState() on the theory that the writeability of sessionStorage might change from page load to when we try writing to it later (if in the interim lots of data ends up getting written to sessionStorage). Thoughts?

Testing Done

  • Ran /demo tour in Safari private mode. Verified state was being written to cookie and that no console errors were thrown.
  • Existing Mocha tests pass.

@kate2753
Copy link
Contributor

The fix looks good to me. I could not find any API that we can rely on to check available storage space, so try\catch seems like a way to work around this.

I agree, we need to have try\catch in "setState" method as well, since availability of sessionStorage may change.

@kate2753
Copy link
Contributor

Code looks good to me. Any luck with unit testing this?

@timlindvall
Copy link
Collaborator Author

No luck, I'm afraid. I've created a potential unit test that passes (https://gist.github.com/zimmi88/80e9d72df993802efa8f) but, because we aren't tearing down and re-initializing Hopscotch between each test, once we trip the sessionStorage flag, we don't have the means by which to flip the switch back to normal. This causes later tests, which assume we're writing to sessionStorage, to fail.

Let's consider adding an issue that minimizes these dependencies between tests. We could also consider building into the build process special targets for unit tests to run against that make the internal methods of Hopscotch available for tests (possibly part of work for #72).

@kate2753
Copy link
Contributor

kate2753 commented Jun 5, 2014

Sounds good. I'll merge this in tomorrow by EOD if there are no objections.

kate2753 added a commit that referenced this pull request Jun 5, 2014
Fix sessionStorage exceeds quota error in Safari
@kate2753 kate2753 merged commit cfd6044 into LinkedInAttic:master Jun 5, 2014
@timlindvall timlindvall deleted the storageErr branch June 5, 2014 20:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants