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

Use CookieStorage when accessing localStorage throws an error #698

Merged
merged 4 commits into from
Mar 8, 2018

Conversation

luisrudge
Copy link
Contributor

some browsers throw an error when trying to access localStorage when localStorage is not available

@luisrudge luisrudge added this to the v9.3.3 milestone Mar 8, 2018
@@ -201,7 +201,7 @@ <h2>Console:</h2>

var webAuth = new auth0.WebAuth({
domain: 'brucke.auth0.com',
redirectUri: 'https://localhost:3000/example',
redirectUri: 'https://localhost:3000/example/',
Copy link
Member

Choose a reason for hiding this comment

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

Why has this URL changed now?

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 was trying it in safari and it doesn't accept the #access_token if you don't have a / in the end 🤦‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tested in other browsers to make sure this addition doesn't do something silly with this addition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

chrome/safari/firefox. here's the thing: we already have code that handles when you can't read/write from the storage, the issue was that safari specifically was throwing an exception even before that (just by referencing localSTorage).

@@ -5,7 +5,14 @@ var Warn = require('../warn');

function StorageHandler() {
this.warn = new Warn({});
this.storage = windowHandler.getWindow().localStorage || new CookieStorage();
Copy link
Member

Choose a reason for hiding this comment

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

The PR name is use cookiestorage when local not available. It already did this though? This is a fix to catch errors no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed

@luisrudge luisrudge changed the title Use CookieStorage when localStorage is not available Use CookieStorage when accessing localStorage throws an error Mar 8, 2018
// when localStorage is disabled.
this.storage = windowHandler.getWindow().localStorage;
} catch (e) {
this.warn.warning("Can't use localStorage. Using CookieStorage instead.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you want to add the e to the log?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@joshcanhelp joshcanhelp left a comment

Choose a reason for hiding this comment

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

LGTM

@luisrudge luisrudge merged commit 9ae72ff into master Mar 8, 2018
@luisrudge luisrudge deleted the feature-new-storage branch March 8, 2018 19:44
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.

4 participants