basket.js

basket.js copied to clipboard

There are several reads from localStorage that are done without try/catch. I see "Permission Denied" exceptions in our logs, I'm guessing from cautious users who have their localstorage disabled.

It could automatically do a standard require instead of throwing an exception... I'd be nice to not have to worry about having the exception thrown and rendering the page useless.

I have personally never run into the permission denied issue with localStorage here, but it wouldn't surprise me if this happened in a small number of circumstances. I'm not opposed to us factoring it in to our API, but would like to learn more about the data driving the ask. Did your logs mention if these were users from a particular browser?

Some errors:

• Firefox 29, localStorage is null, in basket.get.
• IE 11: Access is denied. in basket.clear.

Thanks for looking into this. If you need more errors/browsers let me know and I'll take a deeper look into our Sentry logs.

I also get an error related to this when I check the "Block any sites from setting data" option in Chrome. The error is Uncaught SecurityError: Failed to read the 'localStorage' property from 'Window': Access is denied for this document.

This prevents that any code called via basket will fail to load and thus will break the site.

Adding try/catch eg. https://gist.github.com/justmarkup/c1c9da55d4c84fcca084 would solve this problem. Question is, does anything speak against using try/catch? I am happy to make a PR, but want to make sure you are fine with this change?

My only concern with try/catch is V8 has limited ability to optimise functions using this construct. If someone wants to throw together a PR we can profile and check if this will actually be an issue.

Hi @addyosmani

I just created a new branch on my fork to compare the performance, see: https://github.com/justmarkup/basket.js/commit/256161ad13a6ed7b0c89769efb7b246b284fef70

I am not adding a try/catch block inside any of the functions as I think this helps with the V8 performance issue.

Test with current code: https://justmarkup.com/tests/local.html Test with my try/catch: https://justmarkup.com/tests/local-trycatch.html

I don't really know how to profile this so would be great if you could point me in the right direction or help me out here. Thanks.

I think this is a serious issue. Testing basketjs on my IE11 I had this error: The code on this page disabled back and forward caching. For more information, see http://go.microsoft.com/fwlink/?LinkID=291337. Then some Access is denied caused by basket.js. The whole site obviously was messed up.

A search on the Web drove me to this Stackoverflow answer: http://stackoverflow.com/a/20848924. This was exactly my case: after correcting the integrity level of the %userprofile%\Appdata\LocalLow the localstorage begun to works good.

Now, before execute basketjs, I test localstorage[1]:

function lsTest(){
    var test = 'test';
    try {
        localStorage.setItem(test, test);
        localStorage.removeItem(test);
        return true;
    } catch(e) {
        return false;
    }
}

if(lsTest() === true){
    console.log('presente');
}else{
    console.log('assente');
}

[1] http://stackoverflow.com/a/16427747