Set options.dirty to true if reading locally due to dirty collection

[elad]

The documentation states that if a collection is dirty, fetch will read from localStorage. This is fine, but similar to how other places set options.dirty to true to indicate the operation happened locally, I feel the same should happen here. Then the pattern of testing options.dirty in the success callback is consistent across all CRUD methods.

It looks like this is a simple change in dualsync to add options.dirty = true below the if localsync('hasDirtyOrDestroyed', model, options) line.

Comments?

[nilbus]

I'm in favor of this.

[elad]

Here's a diff: elad/Backbone.dualStorage@master...remote_local_fixes

The base is my master branch which already has the fixes discussed in the other issue (offline status handling) since I need it for my app.

What it does:

• Adds options.dirty = true for "dirty" reads from localStorage as suggested above
• Exposes hasDirtyOrDestroyed, a function returning true if the collection has any dirty/destroyed models or false otherwise
• Adds discard, a function that takes a string (dirty/destroyed) or nothing (both) and discards any such models
• Allows specifying remote or local in the options for individual requests. The use case here for me is importing data. It doesn't make sense to save it to localStorage and say the operation was successful since the very nature of the operation is to persist the data to remote storage.

[elad]

Thanks! (I was about to look into this today since I remembered seeing this feature documented but not implemented!)

[nilbus]

Hang tight, I'm rewriting the merge since I messed something up. :P

[nilbus]

I cherry-picked the options.dirty commit and fixed it so that options.dirty gets set when the browser is offline (related to #104).

I think it would be better to implement hasDirtyOrDestroyed and discard in terms of Store, rather than using localStorage directly. Store already has a hasDirtyOrDestroyed method you can use, and you can move the implementation of discard there too. It's important to isolate dependencies like localStorage in a single class; this is Store's purpose. #73 actually swaps out localStorage in favor of IndexedDB; resolving this conflict will be easier if only Store references localStorage.

[nilbus]

Also for these methods please add integration tests (use the external API and don't mock anything) so that we can ensure they continue to work after the backend is switched.

[elad]

You're right about the importance of abstraction here, I completely forgot about the 2.5mb localStorage limit... I'll take care of it and add the relevant tests.

By the way, I'm not sure hasDirtyOrDestroyed is necessary - I ended up not using it and prefer to keep the API as clean as possible.

[nilbus]

okay​. Feel free to remove that.

[nilbus]

Released in 1.3.0

[elad]

Hey,

Sorry for getting back to this after so long...

When you talk about implementing discard and hasDirtyOrDestroyed in terms of Store, how do you suggest exposing these functions? collection.sync('clear', null, { local: true });?

(I'm asking because the implementation of all functions on Backbone.Collection directly access localStorage...)

[nilbus]

I wouldn't want to expose anything new on the collection. My thought was to make Store the only thing that references localStorage and the only thing that knows about the suffixes '_dirty' and '_destroyed'. Then this knowledge is in one place instead of being spread out across all the code. Store would need to have a couple other methods such as dirtyRecords and destroyedRecords that would return the corresponding list of ids for the current store.

With Store implemented like this, then all the collection methods can be implemented as such, not needing to know about localStorage or the suffixes:

dirty = new Store(getStoreName(this)).dirtyRecords()
ids = (dirty and dirty.split(',')) or []

[elad]

Do you mean something along these lines?

Collection:

Backbone.Collection.prototype.dirtyRecords = ->
  new Store(getStoreName(this)).dirtyRecords()

Backbone.Collection.prototype.destroyedRecords = ->
  new Store(getStoreName(this)).destroyedRecords()

Backbone.Collection.prototype.hasDirtyOrDestroyed = ->
  new Store(getStoreName(this)).hasDirtyOrDestroyed()

Backbone.Collection.prototype.discard = (what) ->
  new Store(getStoreName(this)).discard(what)

Store:

  # Return an array of dirty model ids.
  dirtyRecords: ->
    @recordsOn @name + '_dirty'

  # Return an array of destroyed model ids.
  destroyedRecords: ->
    @recordsOn @name + '_destroyed'

  # Discard dirty and/or destroyed models.
  discard: (what) ->
    if not what or what is 'dirty'
      ids = @dirtyRecords()
      for id in ids
        localStorage.removeItem @name + @sep + id
      localStorage.setItem @name + '_dirty', []
    if not what or what is 'destroyed'
      ids = @destroyedRecords()
      for id in ids
        localStorage.removeItem @name + @sep + id
      localStorage.setItem @name + '_destroyed', []
    @

(edited to add missing functions discussed above.)