Reverting changes from #2003 and 1f3f45252f

[tgriesser]

The changes in 1f3f452 were made to consolidate the error and success handlers to be called/triggered within Backbone.sync function. This change makes the other collection/model methods DRY-er internally, but it also encapsulates the event triggers within Backbone.sync, making it more difficult to create custom sync handlers directly on the model/collection for cases where the default Backbone sync behavior isn't desired.

A good example would be a case where an application is interacting with multiple api's at once - where most models are using a traditional path with correct http codes and response formats, but one is passing through a json-p endpoint where everything returns a 200, and the error code/response is contained in the response.

Previously you'd only need to extend sync like so:

model.sync: function (method, model, options) {
   var success = options.success;
   return Backbone.sync(method, model, _.extend(options, {
     dataType: 'jsonp',
     success: function (data, status, xhr) {
        options.success = success;
        var method = (data.code !== 200) ? 'error' : 'success';
        options[method](data.content);
     }
   });
}

now, you need to also take care of the trigger, and prevent the "sync" event/success handlers which are now is now called internally. To get the same functionality you'd effectively need to add something like this:

var methodMap = {
  'create': 'POST',
  'update': 'PUT',
  'patch':  'PATCH',
  'delete': 'DELETE',
  'read':   'GET'
};
var urlError = function () {
  throw new Error('A "url" property or function must be specified');
};
model.sync: function (method, model, options) { 
   var success = options.success;
   return $.ajax(_.extend(options, {
     type: methodMap[method],
     url: _.result(model, 'url') || urlError(),
     contentType: 'application/json',
     dataType: 'jsonp',
     success: function (data, status, xhr) {
        var method = (data.code !== 200) ? 'error' : 'success';
        options[method](data.content);
        options.success = success;
        if (method === 'error') {
          model.trigger('error', model, xhr, options);
        } else {
          model.trigger('sync', model, resp, options);
        }
     }
   }));
}

The same functionality which was just a few lines previously is now much more work to patch, basically requiring a new sync implementation - and if more functionality is moved at some point out of the individual methods and into the Backbone.sync handler, the developer needs to add more code to create these custom handlers. The Backbone.wrapError (while undocumented as a public API) served as a convenient place to centralize any error logic, keeping everything else contained in the methods themselves.

The changes from #2003 were aiming to standardize the arguments to make Backbone more "agnostic" with regards to the sync handler, but it seems to complicate the api unnecessarily - of the (resp, status, xhr), the resp is the only argument used by Backbone internally. So as long as the first argument is a valid object, any type of sync handler can be dropped in without issue.

I could go for keeping 1f3f452 if the argument for keeping these pieces outside of the "Backbone.sync" handler isn't strong enough, but #2003 seems to be a step backward for keeping the "sync" simple, with the need to pass along an options object and model rather than shadowing them from the function calling sync.

Related: #2161, #2140, #2031, #2145, #2094

If this is something we want to eventually merge, I can update the docs as well.

/cc @kpdecker

[jashkenas]

I dunno about this -- isn't there a way that we can keep things DRY and support the hooks required for folks to be able to extend things comfortably?

[mikeangstadt]

Can't we just enhance plugin extendability as a way to extend functionality w/o stink hooks?

Sent via BlackBerry by AT&T

-----Original Message-----
From: Jeremy Ashkenas notifications@github.com
Date: Mon, 21 Jan 2013 13:52:17
To: documentcloud/backbonebackbone@noreply.github.com
Reply-To: documentcloud/backbone reply@reply.github.com
Subject: Re: [backbone] Reverting changes from #2003 and 1f3f452 (#2173)

I dunno about this -- isn't there a way that we can keep things DRY and support the hooks required for folks to be able to extend things comfortably?

---

Reply to this email directly or view it on GitHub:
#2173 (comment)

[caseywebdev]

After trying out multiple approaches to this, I still think the current implementation is correct. The solution to your example is pretty simple here:

var sync = Backbone.sync;
Backbone.sync = function (__, model, options) {
  if (model.jsonp) _.extend(options, {dataType: 'jsonp'});
  return sync.apply(this, arguments);
};

var ajax = Backbone.ajax;
Backbone.ajax = function (options) {
  var success = options.success;
  options.success = function (resp, __, xhr) {
    if (options.dataType !== 'jsonp') return success(resp);
    var meta = xhr.meta = resp.meta;
    var data = xhr.data = resp.data;
    if (meta.status < 300) return success(data);
    options.error(xhr);
  };
  return ajax.apply(this, arguments);
};

var GitHubUser = Backbone.Model.extend({
  url: 'https://api.github.com/users',
  jsonp: true
});

var MyUser = Backbone.Model.extend({
  url: '/users'
});

[braddunbar]

From the docs:

Backbone.js gives structure to web applications by providing models with key-value binding and custom events, collections with a rich API of enumerable functions, views with declarative event handling, and connects it all to your existing API over a RESTful JSON interface.

I understand that not all service layers are RESTful. However, I don't think that the default Backbone.sync can be appropriate for all of them. It certainly doesn't pass the 90% litmus test and providing a custom Backbone.ajax or Backbone.sync provides a greater degree of control anyway.

As for #2003, I think that (model, resp, options) is a more transport agnostic API. It's doesn't exactly fit $.ajax, but I don't think that it has to because they are not the same thing. Backbone is for abstracting $.ajax, not for mimicking it.

[tgriesser]

@jashkenas - it wasn't really the hooks I was concerned with (those can be private for that matter), it's more the fact that keeping success/error handlers inside the Backbone.sync makes the functionality defined by the collection/model methods dependent on the Backbone.sync internally, and therefore a little trickier to override with a custom model.sync than was possible in previous Backbone-s.

@caseywebdev's solution seems to work, and after talking a little more with @braddunbar, it sounds like I might be in the minority on that one.

I'm not entirely sold on #2003, I don't quite see how it makes sync much more transport agnostic than it was previously (have any sync implementations had issue with this before?) and it doesn't seem to pass the 90% litmus test for cases where someone isn't using jQuery. This is a good example of where it ends up adding an extra step.

[tgriesser]

Closing in favor of #2221.