documentcloud/backbone — pull request #2173 by tgriesser (2013-01-21 21:36:58+00:00)
Title: Reverting changes from #2003 and 1f3f45252f

The changes in 1f3f45252f 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:
```js
model.sync: function (method, model, options) {
var success = options.success;
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:
```js
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;
$.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);
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](https://github.com/documentcloud/backbone/pull/2005/files#L2L677) 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