nilbus / Backbone.dualStorage

A dual (localStorage and REST) sync adapter for Backbone.js
Other
798 stars 108 forks source link

Remote fail on model fetch (read) from previously-stored collection results in malformed storeName string #38

Closed lyzadanger closed 11 years ago

lyzadanger commented 11 years ago

Hi!

I have found that the combination of a certain series of events causes a predictable issue with the localStorage key generated by window.Store.find():

  1. Fetch a collection from remote successfully. dualStorage is successful with localSync in creating localStorage copies of each of the individual contained models in the collection. Yay!
  2. Attempt to fetch a model later from remote that is part of the previously successfully-fetched collection.
  3. Remote fail/error*. dualStorage then calls localSync, which attempts to retrieve that model from localStorage. (The remote error is an expected failure, as the models contained in the collection haven't been persisted yet remotely).
  4. find in window.Store always appends the model.id to the existing storeName (@name). But in this case, the model's ID is already on the storeName.
  5. Results in the model's id repeating twice in the resulting key it attempts to look up in localStorage, which of course fails.

So, in my case, it tries to retrieve key http://myapi.com/api/v1/wines/840014200899001000000001840014200899001000000001

instead of the correct

http://myapi.com/api/v1/wines/840014200899001000000001

(My IDs are long, yep; they're not actually mine).

My workaround for now is a bit hackish and may not take into account subtleties. But I have a habit of not getting around to opening issues if I try to find a perfect solution :).

  find: (model) ->
    storeKey = @name
    if model and model.id
      re = new RegExp "\/#{model.id}$"
      if not re.exec @name
        storeKey += @sep + model.id
    JSON.parse localStorage.getItem(storeKey)
nilbus commented 11 years ago

Thanks, I'm glad you found this. I'd rather figure out why @name is getting an id appended, rather than look it up by the doubled id, since that should not be happening. I can take a look at this later if you don't figure it out first.

lyzadanger commented 11 years ago

@nilbus You are so right! I took some time and tracked this down more thoroughly. I needed to catch the badness on the storeName, not try to piecemeal fix the glitch on the individual methods and the way they generate the key for localStorage. I found two separate issues. The first:

I use urlRoot on some of my models so they can be used outside of collections. This seems to be the main root of the problem. I was able to effect a fix in my own codebase by changing the following line in the dualsync method:

options.storeName = result(model.collection, 'url') || result(model, 'url')

to

options.storeName = result(model, 'urlRoot') || result(model.collection, 'url') || result(model, 'url')

That fixed the main problem (model.id duplication on keys).

Second, more minor issue is that if one doesn't have a trailing slash on one's url (or urlRoot) return values, dualStorage will not add one, which results in store keys that look like:

http://myapi/v1/posts3

instead of

http://myapi/v1/posts/3

I've worked around this by adding trailing slashes to the URLs of all of my models and collections, but it seems like it would be nice to not have to do this :).

nilbus commented 11 years ago

Backbone.Model provides a url method that your models should have, which appends a / as appropriate and the model's id. See http://backbonejs.org/#Model-url.

I think the issue here might be that both your collection and model define a url method, and it's going with the collection's version first.

Also watch out and make sure that your model's url contains an id, the model's urlRoot does not, and that the collection url does not. If either model urlRoot or collection url had an id in it, you would see duplicated IDs too.

Let me know if this resolves your issue, or if there's still something going on that we need to address.

nilbus commented 11 years ago

Closing this since I didn't hear back from you, but definitely let us know if there's an issue for us to address.