jeromegn/Backbone.localStorage

Problem with backbone.localstorage on some Androids

Closed this issue · 6 comments

Hi Jerome!

Found problem with methods .find of your backbone.localstorage plugin on few Androids (Android 2.3.5 Samsung Galaxy S2 and some other).
The problem connected with JSON.parse(null) of find method. With null - it cause an error "Uncaught illegal acces".

Could propose a fix with validation before parsing:

// Retrieve a model from this.data by id.
find: function(model) {
var storageData = this.localStorage().getItem(this.name+"-"+model.id);
if (storageData) {
//storageData null cause error of JSON.parse(null) on some androids
return JSON.parse(storageData);
} else {
return null;
}
},

// Return the array of all models currently in storage.
findAll: function() {
return _(this.records).chain()
.map(function(id){
var storageData = this.localStorage().getItem(this.name+"-"+id);
if (storageData) {
//storageData null cause error of JSON.parse(null) on some androids
return JSON.parse(storageData);
} else {
return null;
}
}, this)
.compact()
.value();
},

confirming this is a bug as well.

Same here. Fix from Sergeyshall resolved the issue for me.

Wow, I have no been receiving the email notifications of new issues. I'm sorry if that took a while.

Would you mind submitting a pull request with that code? Maybe even write a test for it :)

My two cents - I've come across this problem before too, but my suggested approach would be to duck-punch JSON.parse. This way we don't need to add the same null checks every time we are parsing JSON. Pull Request #41 only fixes the case of JSON.parse within this library - what if I need to do JSON.parse in my own code?

For example:

brianleroux/lawnchair#48 (comment)

We've come across this issue as well. In our own code it's trivial to implement a null check like value && JSON.parse(value);. To keep the fixing code separate we added jsonData().

find: function(model) {
  // call to this.jsonData instead of JSON.parse
  return this.jsonData(this.localStorage().getItem(this.name+"-"+model.id));
}

findAll: function() {
  return _(this.records).chain()
    .map(function(id){
      return this.jsonData(this.localStorage().getItem(this.name+"-"+id));
    }, this)
    .compact()
    .value();
}

and as above the actual handling is simple

jsonData: function (data) {
  return data && JSON.parse(data);
}

@nerdgore If you'd like to submit that as a pull request, it'd be greatly appreciated.