Backbone.sync: inconsistent arguments to options.success and options.error

[PaulUithol]

Having some issues with this in our app, related to 1f3f452 .

It seems inconsistent to me to call options.success with the original arguments passed to it by jQuery (resp, status, xhr -> success(resp, status, xhr)), and to change the signature for the error callback (xhr, status, thrown -> error(model, xhr, options)?

    var success = options.success;
    options.success = function(resp, status, xhr) {
      if (success) success(resp, status, xhr);
      model.trigger('sync', model, resp, options);
    };

    var error = options.error;
    options.error = function(xhr, status, thrown) {
      if (error) error(model, xhr, options);
      model.trigger('error', model, xhr, options);
    };

Shouldn't one of these styles be chosen? Either preserve the jQuery call signature, or convert it to the same style as the model events?

[caseywebdev]

Yes these inconsistencies are not ideal. I vote stick with the Backbone model style callbacks to stay as agnostic as possible.

if (success) success(model, resp, options);

(xhr is available in options.xhr now if needed)

[braddunbar]

This seems like a fine change to me since the (resp, status, xhr) style is only exposed to sync, not save/fetch/etc. In fact, status is not even used by Backbone internally.

@PaulUithol Is this for a custom sync override?

[tgriesser]

I like @caseywebdev's change as well, keeping the model as the first argument for both the callbacks and the events would be much cleaner

[PaulUithol]

@braddunbar Yes, it is. I don't really have a preference for either resolution (although always having the related model would be nice), just noticed the discrepancy when we got some errors from an old plugin that expected xhr as the first argument.

[jashkenas]

Sounds good to me -- feel free to do a PR and merge the change.

[braddunbar]

Coming right up.

[braddunbar]

Fixed in #2005.

[kpdecker]

How does one access the remaining error handler options with the code changes in #2005? Why doesn't complete have the same API as success and error?

[braddunbar]

What options do you need to access? The xhr is stored in options.xhr, and I think everything is accessible from there. I'm not sure what you mean by "complete" above though. Would you mind expanding on that?

[braddunbar]

@kpdecker I realized after posting this that you are referring to the complete option to $.ajax. Backbone only handles success and error. complete is just passed through as is to $.ajax.

I hadn't considered this before, but I think it's alright that we pass it through. It's also documented that way.

[kpdecker]

@braddunbar Under Zepto error can include additional data explaining the exact error if it was an issue above the HTTP stack in the AJAX stack ex:

settings.error.call(context, xhr, type, error)

Where type can be things like timeout, parsererror, and others.

With the error handler that is in place now this error information is silently discarded and there is no way for us to access it short of zepto global ajax events which are not without their drawbacks.

Regarding the complete handler, it seems like it should be consistent one way or the other with the other AJAX callbacks, either augment or do not augment the parameters with the data object.

[braddunbar]

Regarding the extra Zepto argument, the previous implementation didn't provide the argument either, right?

As for complete, I think providing the same signature would be fine, though this can also be accomplished via Backbone.ajax, which should probably be the preferred way to implement things that Backbone doesn't use internally.

[kpdecker]

The previous implementation exposed the RAW Zepto endpoints, on a per-instance basis via sync:

  options.error = Backbone.wrapError(options.error, model, options);
  return (this.sync || Backbone.sync).call(this, 'read', this, options);

Note that the options.error assignment is done before a point that the instance can override behavior so under that implementation it's still possible for wrapping of Backbone.sync on a per-instance basis.

On the complete side I think the question is one of "is this an api" or "is this internal code that they are mucking with". If the former I would argue for API consistency between the three cases that are provided. If the later then all of my arguments are kind of pointless as it's not safe to implement against anyway :)