elgerlambert / redux-localstorage

Store enhancer that syncs (a subset) of your Redux store state to localstorage.
MIT License
1.32k stars 107 forks source link

App won't load with blocked cookies #50

Closed jonathanasquier closed 8 years ago

jonathanasquier commented 8 years ago

Hi, I had a weird behaviour on a customer's mobile device : cookies where blocked in safari and the whole app wouldn't load and triggered this error from redux-localstorage: DOM Exception 18: An attempt was made to break through the security policy of the user agent. Seems to be related to accessing localstorage, I don't know if it is by design or if it should fail without breaking the app.

thanks, great work

elgerlambert commented 8 years ago

Hi @jonathanasquier,

Weird indeed! I wasn't aware of this issue. Having just googled your error message I came across the following: http://apple.stackexchange.com/questions/125584/broken-safari-dom-exception-18 Seems to describe the exact same issue as the one you're having. Although I find it strange it appears to be expected behaviour (or a known issue with Safari - depending on how you look at it).

I'm able to reproduce what you're seeing using 1.0.0-x, which has a try catch directly around the call to localstorage, since that doesn't catch the error it doesn't appear that the error can be handled. I also tried it using localForage, it had the same problem; app doesn't load. What's worse is that with localForage it doesn't print any error to console, but turning cookies back on fixes the issue so that's definitely the cause. So it appears there's not a lot we can do about it, other then file an issue with Safari perhaps.

jonathanasquier commented 8 years ago

Okay, so I understand it's an issue with safari's localstorage implem? I'm not really confortable in releasing an app which I can't stop from breaking in certain conditions but if there is not much to be done then... But what is preventing the try catch block from catching this error?

Thank you for your feedback :)

elgerlambert commented 8 years ago

I did some further digging, there may well be a way to catch this error and prevent it from crashing your app.

I expected the error to be thrown when an attempt was made at setting or getting an item from storage, turns out the exception is thrown as soon as window.localStorage is accessed (which persistState.js does, without a try/catch block around it).

Which version of redux-localstorage are you using (0.4.x or 1.0.0-x)? If 1.0.0-x, do you provide your own (enhanced) storage or do you depend on the default (localStorage)?

jonathanasquier commented 8 years ago

I'm using 1.0.0-rc4 and the default window.localstorage :

const storage = compose(
    filter('auth')
)(adapter(window.localStorage));

var composed = [
    //...
    persistState(storage)
];

//store
compose(...composed)(createStore)(reducer)

Would there be a way to test persistState(storage) before adding it (or not) to the composed array before making the store?

Thanks

elgerlambert commented 8 years ago

Okay, since you provide your own enhanced storage to persistState, you'll have to put a try/catch around window.localStorage in some way.

The following is untested (and isn't pretty to look at), but something like:

let storage; 

try {
  storage = compose(
    filter('auth')
  )(adapter(window.localStorage));
} catch (e) {
  storage = {
    get(){},
    put(){},
    del(){}
  }
}

Also, make sure you upgrade to rc5 (it includes a fix that also affects your issue).

jonathanasquier commented 8 years ago

The try catch bloc caught the exception, but I still got a blank page when setting an empty storage. I chose to test whether storage is undefined to push it in the compose array:

let storage;

try {
    storage = compose(
        filter('auth')
    )(adapter(window.localStorage));
} catch(e) {

}

var composed = [
    //...
];

if(storage)
    composed.push(persistState(storage));

//store
compose(...composed)(createStore)(reducer);

Works like a charm! Thanks!

elgerlambert commented 8 years ago

Hi @jonathanasquier,

Glad to hear you've found a way!

I'll look into adding a try/catch to the adapter(window.localStorage) used internally (when no storage argument is specified). That should help make your code a little cleaner as it should take care of the "empty storage" scenario you described.