IE7 does not fail gracefully #85

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

remmons commented Apr 19, 2013

IE7 does not fail gracefully unless you do a local storage feature detection. A message of "'localstorage' is undefined" appears in the error console without the following code.

+ storage.removeItem(uid);
+ return result && storage;
+ } catch(e) {}
+}());
@jeromegn

jeromegn May 11, 2013

Owner

I'm sure there's a simpler implementation. The new Date bit doesn't serve much of a purpose.

Maybe inspire from this? https://github.com/Modernizr/Modernizr/blob/master/feature-detects/storage/localstorage.js

@qivhou

qivhou May 12, 2013

I'm just thinking that we don't need the hasLocalStorage(), since it's easy to know if the window.localStorage supported or not by following code

if(window.localStorage){
...//it's supported by current browser

}

since we already get the reference of window.localStorage by Backbone.localStorage()
so just added following and remove the code added by @remmons

Backbone.LocalStorage = window.Store = function(name) {
  this.name = name;
  //Check if window.localStorage supported
  if (this.localStorage()) {
    var store = this.localStorage().getItem(this.name);
    this.records = (store && store.split(",")) || [];
  }
};
@jeromegn

jeromegn May 12, 2013

Owner

Yea I think that would be good enough. Ideally though, since we're doing it, I'd like it to be more bulletproof. Modernizr usually has the good bits.

My concern is this:

  • Should we throw an error if localStorage isn't supported (and there is rendering this plugin useless) and then leave it to the developer to handle this case?
  • Or should we have it fail silently like the code in this pull request?
- return localStorage;
+ if (hasLocalStorage) {
+ return localStorage;
+ }
@jeromegn

jeromegn May 11, 2013

Owner

Looks like if window.localStorage wasn't defined, then it would return undefined. But since you're checking before, if window.localStorage is not supported, it would return null.

Not sure I see the point of this code.

Owner

jeromegn commented May 11, 2013

Do we even want it to fail gracefully? Maybe we want to throw an error so that an error reporter would catch it.

What's the problem with it adding an error to the console's log?

Contributor

yakubori commented May 12, 2013

There is nothing wrong, imho, with throwing something like the following, and leaving it to the user to decide whether to wrap their call in a try/catch.

Backbone.LocalStorage = window.Store = function(name) {
  if( !this.localStorage ) {
    throw "Backbone.localStorage: Environment does not support localStorage."
  }
  this.name = name;
  var store = this.localStorage().getItem(this.name);
  this.records = (store && store.split(",")) || [];
};
Owner

jeromegn commented May 13, 2013

@yakubori I like it.

yakubori pushed a commit to yakubori/Backbone.localStorage that referenced this pull request May 13, 2013

Check for localStorage support
Proposed fix for Issue #85
Owner

jeromegn commented May 19, 2013

Decided to go with #90 which makes more sense. I want it to fail loudly and quickly.

@jeromegn jeromegn closed this May 19, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment