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

Framework: Initialize localStorage polyfill at start of boot #6558

Merged
merged 1 commit into from
Jul 6, 2016

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Jul 6, 2016

Fixes #6555
Suspected regression in #6236 (lib/abtest pulls in store)

This pull request seeks to resolve an issue where Safari Private Mode does not load correctly because of an attempt to access localStorage (not available). Our localStorage polyfill is targeted at resolving this case, but must be loaded before any dependencies making use of store or localStorage. This pull request moves the initialization to the top of the boot file to ensure that it is always executed first.

Testing instructions:

I was never able to reproduce the original issue in development mode locally, perhaps due to some configuration flag that's only active in production (makes sense if it is ABTest related). You may want to search internal documentation for running production mode locally to verify the fix (which I did to verify resolution), else ensure that no regressions occur in using the application. Specifically, ensure that you can load Calypso in Safari Private Mode.

/cc @rachelmcr, @bisko, @blowery, @mjuhasz

Test live: https://calypso.live/?branch=fix/6555-localstorage-private

@aduth aduth added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 6, 2016
@rachelmcr
Copy link
Member

I confirmed that Calypso loads for me in Safari Private Mode on the calypso.live branch.

@aduth
Copy link
Contributor Author

aduth commented Jul 6, 2016

@rachelmcr : Do you see it being broken at https://calypso.live/?branch=master ?

@bisko
Copy link
Contributor

bisko commented Jul 6, 2016

Confirmed broken on the calypso.live master branch.
Confirmed working on the calypso.live fix/6555-localstorage-private branch.

Unable to reproduce on local Calypso, because it seems that localStorage has a bit of weird behaviour on local sites.

@rachelmcr
Copy link
Member

Hm, I checked the master branch on calypso.live and it's working for me there, so at least on my end I can't confirm the fix based on calypso.live testing.

@aduth
Copy link
Contributor Author

aduth commented Jul 6, 2016

Ref: 2834-gh-calypso-pre-oss (cc @rralian )

@aduth aduth added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 6, 2016
@aduth aduth merged commit 52f8c00 into master Jul 6, 2016
@aduth aduth deleted the fix/6555-localstorage-private branch July 6, 2016 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calypso not loading in Safari private window
4 participants