Add support for setting collection with a promise

[reubano]

Some APIs (flickr) require you to call multiple endpoints before returning useful data. In these cases, you need to return a promise for the data in parseBeforeLocalSave that gets resolved once all the API calls are complete. This change adds support so that the promise is handled properly.

Usage

models/photos.coffee

Collection = require 'models/base/collection'
Model = require require 'models/base/model'
config = require 'config'

module.exports = class Photos extends Collection
  _.extend @prototype, Chaplin.SyncMachine

  base_url = "https://api.flickr.com/services/rest/"
  base_data =
    api_key: config.flickr.api_token
    format: 'json'
    nojsoncallback: 1

  url_data =
    method: 'flickr.urls.lookupUser'
    url: "https://www.flickr.com/photos/#{config.flickr.user}/"

  model: Model
  url: "#{base_url}?#{$.param _.extend url_data, base_data}"
  storeName: 'Photos'
  local: -> localStorage.getItem "synced"

  getCollection: (response) =>
    data =
      method: 'flickr.collections.getTree'
      collection_id: config.flickr.collection_id
      user_id: response.user.id

    $.get base_url, _.extend data, base_data

  getSets: (response) =>
    deferreds = []

    for id in (s.id for s in response.collections.collection[0].set)
      data =
        method: 'flickr.photosets.getPhotos'
        photoset_id: id

      deferreds.push $.get base_url, _.extend data, base_data

    deferreds

  applySets: (deferreds) => $.when.apply($, deferreds)

  getData: (results...) =>  _.flatten (r[0].photoset.photo for r in results)

  parseBeforeLocalSave: (resp) =>
    return if @disposed
    @getCollection(resp).then(@getSets).then(@applySets).then(@getData)

  wrapError: (model, options) =>
    error = options.error
    options.error = (resp) =>
      error model, resp, options if error
      @unSync

  _fetch: (options) =>
    @beginSync()
    options = if options then _.clone(options) else {}
    success = options.success

    options.success = (resp) =>
      method = if options.reset then 'reset' else 'set'
      setData = (data) =>
        @[method] data, options
        success @, data, options if success
        @finishSync()

      try
        resp.done (data) => setData data
      catch TypeError
        setData resp

    @wrapError @, options
    @sync 'read', @, options

  fetch: =>
    $.Deferred((deferred) => @_fetch 
      success: deferred.resolve      
      error: deferred.reject).promise()

Then elsewhere

Photos = require 'models/photos'
photos = new Photos()
photos.fetch().done -> localStorage.setItem "#{config.title}:Photos:synced", true

[nilbus]

I really like where you're going with this.

Are you doing the try/catch to catch the case when resp doesn't have done, or are you trying to catch an error that raises up from inside setResp? I worry that other errors will get caught that we didn't intend to catch. If your goal is making sure resp.done exists, you can check it with:

if resp.done?
  resp.done (data) -> setResp data
else
  setResp resp

I think for tests on this new code, it might be most meaningful to add an integration test to spec/integration_spec.coffee, where you create a collection, call fetch, assert that a promise is returned, and that its done callback is called.

[reubano]

The try/catch is for when resp doesn't have done. When I ran on my machine, the error I got was TypeError. So I guess catch TypeError would be better. Do you think it's still too broad?

[nilbus]

Yes. Later down the road if we call a property or method that doesn't exist in getResp, it'll also catch those. For example:

• collection isn't the collection we expected but is something else because sync was used wrong. Instead of throwing an error that the collection object doesn't have a method get, it will get caught by this try catch.
• If we typo something in the localsync update and call a method/property that doesn't exist, it will get caught by this try catch.

In this case, it's not catastrophic because setResp just gets called again in the catch block, and the error will be thrown again. However it's a good idea to avoid try/catch any time you can do the same thing with an if/else, if for nothing else than performance reasons (we're running that code twice in our case, and the browser has to generate stack traces [somewhat slow] in the general case).

[reubano]

Ok. Fine by me.

[nilbus]

Looking at this at a higher level, I do understand the need to do this:

photos.fetch().done -> ...

What I don't understand is how this change helps accomplish that. fetch returns whatever dualsync/onlineSync/Backbone.sync returns.

I wrote up a spec to see if it was working despite my understanding. It fails with Cannot read property 'done' of undefined

describe 'deferreds', ->
  it 'returns a deferred', ->
    {callbackResponse, deferred} = {}
    runs ->
      model.remote = true
      deferred = model.fetch success: (args...) -> callbackResponse = args
    waitsFor (-> callbackResponse), "The success callback for 'fetch' should have been called", 100
    runs ->
      expect(_.isFunction(deferred.done)).toBeTruthy()

[reubano]

I'll get to your previous questions shortly. This PR doesn't address your spec. See Issue #56 for that.

[nilbus]

Okay, I think I understand the purpose of this then. The issue was that the resp passed to parseBeforeLocalSave was not a deferred object, right?

[reubano]

Almost. parseBeforeLocalSave now returns a promise that you resolved once all the API calls are complete. The resp it receives is still regular JSON. And in this use case, the resp is the flickr id associated with user.