Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Pass along jqXhr, status and errorThrown from jqXhr callbacks #2928

Closed
wants to merge 5 commits into from

3 participants

@c960657

In options.error() it would be useful to get access to the statusText argument passed to jqXhr.error() to know some more about the error.

This patch passes along all parameters from the jqXhr callbacks to callbacks and event handlers.

This was previously suggested in #485, but I don't think it was actually implemented (or it has been changed again later). Simply getting the response object is not sufficient - AFAICT the statusText and errorThrown arguments are not available on the response object. It was also suggested in #2147, but this PR was closed without any discussion.

The order of the arguments is not consistent between success() and error(). This just maintains status quo, but it does make the issue more visible. Not sure what to do about this without breaking BC.

@braddunbar
Collaborator

Hi @c960657! I'm not certain about the error, but statusText is definitely available on the xhr, which is attached to options, and thus already available.

@c960657

Hi @braddunbar!
jqXhr.statusText only reflects HTTP errors.
If the server does not return valid JSON (e.g. due to a fatal error on the server), the textStatus argument for the error() callback contains "parsererror" but statusText is "OK".

@braddunbar
Collaborator

Hi again @c960657. I'm afraid I don't quite follow. Would you mind posting a fiddle/bin with the behavior you're describing?

@c960657

If I load a non-JSON URL with the following debug code, the console contains OK parsererror.

@@ -1583,7 +1583,8 @@
   // Wrap an optional error callback with a fallback error event.
   var wrapError = function(model, options) {
     var error = options.error;
-    options.error = function(resp) {
+    options.error = function(resp, textStatus, errorThrown) {
+      console.log(resp.statusText, textStatus);
       if (error) error(model, resp, options);
       model.trigger('error', model, resp, options);
     };
@braddunbar
Collaborator

Ah! I see now. statusText and textStatus are quite similar and I didn't even notice the difference. :)

@jashkenas
Owner

Is there a way to get the status tacked on to the XHR, instead of as a separate argument?

@jashkenas
Owner

This seems like a case of too many arguments to expose as a public API. Can we condense them down?

@braddunbar
Collaborator

Perhaps we can add statusText to the xhr?

@jashkenas
Owner

That would be ideal. @c960657 — is that the only important property we're currently missing?

@c960657

For completeness I think we should include errorThrown also (then we don't have to define what "important" means :-)).

statusText and errorThrown are the only two arguments passed to the fail handler (apart from the jqXhr object itself).
http://api.jquery.com/jQuery.ajax/#jqXHR

As an alternative to populating the properties on the jqXhr object, we could use the options argument. A few other places in Backbone use the options to pass such, e.g. when passing options.validationError for the 'validate' event. But the two values discussed in this issue are closely related to the jqXhr argument, so populating that is probably preferable in this case.

@jashkenas
Owner

Sounds great. Want to amend this pull request to do exactly that?

@c960657

I'll put it on my todo list :-) Hope to get around to this within a few weeks.

@c960657

I have pushed the code. Still need to update the documentation.

@c960657

Updated the documentation and the failing unit tests. What do you think?

@jashkenas
Owner

A little clunky. Want to dry it up by boiling down the options/xhr juggling into a helper function?

@c960657

I have replaced wrapError() with wrapCallbacks() that handles both success and error callbacks.

@braddunbar
Collaborator

A more succinct version is in #3048. That said, this is pretty specific and can be done fairly easily in a custom sync.

@braddunbar
Collaborator

Fixed by #3048. Thanks!

@braddunbar braddunbar closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 76 additions and 59 deletions.
  1. +31 −34 backbone.js
  2. +15 −5 index.html
  3. +15 −9 test/collection.js
  4. +15 −11 test/model.js
View
65 backbone.js
@@ -456,13 +456,9 @@
options = options ? _.clone(options) : {};
if (options.parse === void 0) options.parse = true;
var model = this;
- var success = options.success;
- options.success = function(resp) {
- if (!model.set(model.parse(resp, options), options)) return false;
- if (success) success(model, resp, options);
- model.trigger('sync', model, resp, options);
- };
- wrapError(this, options);
+ wrapCallbacks(this, options, function(resp, options) {
+ if (!model.set(model.parse(resp, options), options)) return true;
+ });
return this.sync('read', this, options);
},
@@ -500,19 +496,15 @@
// updated with the server-side state.
if (options.parse === void 0) options.parse = true;
var model = this;
- var success = options.success;
- options.success = function(resp) {
+ wrapCallbacks(this, options, function(resp, options) {
// Ensure attributes are restored during synchronous saves.
model.attributes = attributes;
var serverAttrs = model.parse(resp, options);
if (options.wait) serverAttrs = _.extend(attrs || {}, serverAttrs);
if (_.isObject(serverAttrs) && !model.set(serverAttrs, options)) {
- return false;
+ return true;
}
- if (success) success(model, resp, options);
- model.trigger('sync', model, resp, options);
- };
- wrapError(this, options);
+ });
method = this.isNew() ? 'create' : (options.patch ? 'patch' : 'update');
if (method === 'patch') options.attrs = attrs;
@@ -530,23 +522,20 @@
destroy: function(options) {
options = options ? _.clone(options) : {};
var model = this;
- var success = options.success;
var destroy = function() {
model.trigger('destroy', model, model.collection, options);
};
- options.success = function(resp) {
- if (options.wait || model.isNew()) destroy();
- if (success) success(model, resp, options);
- if (!model.isNew()) model.trigger('sync', model, resp, options);
- };
-
if (this.isNew()) {
- options.success();
+ destroy();
+ if (options.success) options.success(model, null, options);
return false;
}
- wrapError(this, options);
+
+ wrapCallbacks(model, options, function(resp, options) {
+ if (options.wait) destroy();
+ });
var xhr = this.sync('delete', this, options);
if (!options.wait) destroy();
@@ -887,15 +876,11 @@
fetch: function(options) {
options = options ? _.clone(options) : {};
if (options.parse === void 0) options.parse = true;
- var success = options.success;
var collection = this;
- options.success = function(resp) {
+ wrapCallbacks(collection, options, function(resp, options) {
var method = options.reset ? 'reset' : 'set';
collection[method](resp, options);
- if (success) success(collection, resp, options);
- collection.trigger('sync', collection, resp, options);
- };
- wrapError(this, options);
+ });
return this.sync('read', this, options);
},
@@ -1656,12 +1641,24 @@
throw new Error('A "url" property or function must be specified');
};
- // Wrap an optional error callback with a fallback error event.
- var wrapError = function(model, options) {
+ // Wrap optional success and error callbacks.
+ var wrapCallbacks = function(model, options, successCallback) {
+ var success = options.success;
var error = options.error;
- options.error = function(resp) {
- if (error) error(model, resp, options);
- model.trigger('error', model, resp, options);
+ options.success = function(resp, textStatus, xhr) {
+ xhr.textStatus = textStatus;
+ options = _.extend({ xhr: xhr }, options);
+ var abort = successCallback.call(this, resp, options);
+ if (!abort) {
+ if (success) success(model, resp, options);
+ model.trigger('sync', model, resp, options);
+ }
+ }
+ options.error = function(xhr, textStatus, errorThrown) {
+ xhr.textStatus = textStatus;
+ xhr.errorThrown = errorThrown;
+ if (error) error(model, xhr, options);
+ model.trigger('error', model, xhr, options);
};
};
View
20 index.html
@@ -865,7 +865,7 @@ <h2 id="Events">Backbone.Events</h2>
<li><b>"destroy"</b> (model, collection, options) &mdash; when a model is <a href="#Model-destroy">destroyed</a>. </li>
<li><b>"request"</b> (model_or_collection, xhr, options) &mdash; when a model or collection has started a request to the server. </li>
<li><b>"sync"</b> (model_or_collection, resp, options) &mdash; when a model or collection has been successfully synced with the server.</li>
- <li><b>"error"</b> (model_or_collection, resp, options) &mdash; when model's or collection's request to remote server has failed. </li>
+ <li><b>"error"</b> (model_or_collection, xhr, options) &mdash; when model's or collection's request to remote server has failed. </li>
<li><b>"invalid"</b> (model, error, options) &mdash; when a model's <a href="#Model-validate">validation</a> fails on the client. </li>
<li><b>"route:[name]"</b> (params) &mdash; Fired by the router when a specific route is matched.</li>
<li><b>"route"</b> (route, params) &mdash; Fired by the router when <i>any</i> route has been matched.</li>
@@ -1228,7 +1228,10 @@ <h2 id="Model">Backbone.Model</h2>
latest server state. A <tt>"change"</tt> event will be triggered if the
server's state differs from the current attributes. Accepts
<tt>success</tt> and <tt>error</tt> callbacks in the options hash, which
- are both passed <tt>(model, response, options)</tt> as arguments.
+ are passed <tt>(model, response, options)</tt> and <tt>(model, xhr, options)</tt>
+ as arguments, respectively. In the success callback, options.xhr contains the
+ jqXHR instance. xhr.statusText, and for error callbacks also xhr.errorThrown, are
+ populated with the corresponding arguments passed to jqXHR callbacks.
</p>
<pre>
@@ -1312,7 +1315,10 @@ <h2 id="Model">Backbone.Model</h2>
<a href="http://api.jquery.com/jQuery.ajax/#jqXHR">jqXHR</a> object, or
<tt>false</tt> if the model <a href="#Model-isNew">isNew</a>. Accepts
<tt>success</tt> and <tt>error</tt> callbacks in the options hash, which
- will be passed <tt>(model, response, options)</tt>.
+ are passed <tt>(model, response, options)</tt> and <tt>(model, xhr, options)</tt>
+ as arguments, respectively. In the success callback, options.xhr contains the
+ jqXHR instance. xhr.statusText, and for error callbacks also xhr.errorThrown, are
+ populated with the corresponding arguments passed to jqXHR callbacks.
Triggers a <tt>"destroy"</tt> event on the model, which will bubble up
through any collections that contain it, a <tt>"request"</tt> event as it
begins the Ajax request to the server, and a <tt>"sync"</tt> event, after
@@ -2059,8 +2065,12 @@ <h2 id="Collection">Backbone.Collection</h2>
<br />
Fetch the default set of models for this collection from the server,
<a href="#Collection-set">setting</a> them on the collection when they arrive.
- The <b>options</b> hash takes <tt>success</tt> and <tt>error</tt> callbacks
- which will both be passed <tt>(collection, response, options)</tt> as arguments.
+ The <b>options</b> hash takes <tt>success</tt> and <tt>error</tt> callbacks,
+ which are passed <tt>(collection, response, options)</tt> and
+ <tt>(collection, xhr, options)</tt> as arguments, respectively. In the success
+ callback, options.xhr contains the jqXHR instance. xhr.statusText, and for error
+ callbacks also xhr.errorThrown, are populated with the corresponding arguments
+ passed to jqXHR callbacks.
When the model data returns from the server, it uses <a href="#Collection-set">set</a>
to (intelligently) merge the fetched models, unless you pass <tt>{reset: true}</tt>,
in which case the collection will be (efficiently) <a href="#Collection-reset">reset</a>.
View
24 test/collection.js
@@ -437,7 +437,7 @@
test("model destroy removes from all collections", 3, function() {
var e = new Backbone.Model({id: 5, title: 'Othello'});
- e.sync = function(method, model, options) { options.success(); };
+ e.sync = function(method, model, options) { options.success(null, 'success', {}); };
var colE = new Backbone.Collection([e]);
var colF = new Backbone.Collection([e]);
e.destroy();
@@ -474,7 +474,9 @@
collection.on('error', function () {
ok(true);
});
- collection.sync = function (method, model, options) { options.error(); };
+ collection.sync = function (method, model, options) {
+ options.error({}, 'error', 'Internal Server Error');
+ };
collection.fetch();
});
@@ -487,7 +489,7 @@
};
collection.url = '/test';
collection.fetch();
- this.syncArgs.options.success();
+ this.syncArgs.options.success({}, 'success', {});
equal(counter, 1);
});
@@ -775,7 +777,7 @@
}
};
col.sync = m.sync = function( method, collection, options ){
- options.success(collection, [], options);
+ options.success({}, 'success', {});
};
col.fetch(opts);
col.create(m, opts);
@@ -784,7 +786,9 @@
test("#1412 - Trigger 'request' and 'sync' events.", 4, function() {
var collection = new Backbone.Collection;
collection.url = '/test';
- Backbone.ajax = function(settings){ settings.success(); };
+ Backbone.ajax = function(options) {
+ options.success({}, 'success', {});
+ };
collection.on('request', function(obj, xhr, options) {
ok(obj === collection, "collection has correct 'request' event after fetching");
@@ -808,7 +812,9 @@
test("#1447 - create with wait adds model.", 1, function() {
var collection = new Backbone.Collection;
var model = new Backbone.Model;
- model.sync = function(method, model, options){ options.success(); };
+ model.sync = function(method, model, options) {
+ options.success(null, 'success', {});
+ };
collection.on('add', function(){ ok(true); });
collection.create(model, {wait: true});
});
@@ -871,7 +877,7 @@
})
});
new Collection().fetch();
- this.ajaxSettings.success([model]);
+ this.ajaxSettings.success([model], 'success', {});
});
test("`sort` shouldn't always fire on `add`", 1, function() {
@@ -1151,7 +1157,7 @@
}));
var ajax = Backbone.ajax;
Backbone.ajax = function (params) {
- _.defer(params.success);
+ _.defer(params.success, {}, 'success', {});
return {someHeader: 'headerValue'};
};
collection.fetch({
@@ -1218,7 +1224,7 @@
strictEqual(resp, 'response');
}
});
- this.ajaxSettings.success('response');
+ this.ajaxSettings.success('response', 'success', {});
});
test("#2612 - nested `parse` works with `Collection#set`", function() {
View
26 test/model.js
@@ -430,7 +430,7 @@
if (attrs.admin) return "Can't change admin status.";
};
model.sync = function(method, model, options) {
- options.success.call(this, {admin: true});
+ options.success({admin: true,}, 'success', {});
};
model.on('invalid', function(model, error) {
lastError = error;
@@ -453,7 +453,7 @@
ok(true);
});
model.sync = function (method, model, options) {
- options.error();
+ options.error({}, 'error', 'Internal Server Error');
};
model.save({data: 2, id: 1});
model.fetch();
@@ -477,7 +477,7 @@
test("save in positional style", 1, function() {
var model = new Backbone.Model();
model.sync = function(method, model, options) {
- options.success();
+ options.success({}, 'success', {});
};
model.save('title', 'Twelfth Night');
equal(model.get('title'), 'Twelfth Night');
@@ -486,8 +486,8 @@
test("save with non-object success response", 2, function () {
var model = new Backbone.Model();
model.sync = function(method, model, options) {
- options.success('', options);
- options.success(null, options);
+ options.success('', 'success', {});
+ options.success(null, 'success', {});
};
model.save({testing:'empty'}, {
success: function (model) {
@@ -763,7 +763,7 @@
deepEqual(JSON.parse(this.ajaxSettings.data), {x: 3, y: 2});
equal(model.get('x'), 1);
equal(changed, 0);
- this.syncArgs.options.success({});
+ this.syncArgs.options.success({}, 'success', {});
equal(model.get('x'), 3);
equal(changed, 1);
});
@@ -778,7 +778,7 @@
test("#1030 - `save` with `wait` results in correct attributes if success is called during sync", 2, function() {
var model = new Backbone.Model({x: 1, y: 2});
model.sync = function(method, model, options) {
- options.success();
+ options.success(null, 'success', {});
};
model.on("change:x", function() { ok(true); });
model.save({x: 3}, {wait: true});
@@ -951,7 +951,7 @@
}
};
model.sync = function(method, model, options) {
- options.success();
+ options.success(null, 'success', {});
};
model.save({id: 1}, opts);
model.fetch(opts);
@@ -960,7 +960,9 @@
test("#1412 - Trigger 'sync' event.", 3, function() {
var model = new Backbone.Model({id: 1});
- model.sync = function (method, model, options) { options.success(); };
+ model.sync = function (method, model, options) {
+ options.success({}, 'success', {});
+ };
model.on('sync', function(){ ok(true); });
model.fetch();
model.save();
@@ -984,7 +986,9 @@
test("#1377 - Save without attrs triggers 'error'.", 1, function() {
var Model = Backbone.Model.extend({
url: '/test/',
- sync: function(method, model, options){ options.success(); },
+ sync: function(method, model, options) {
+ throw "should not be called";
+ },
validate: function(){ return 'invalid'; }
});
var model = new Model({id: 1});
@@ -1007,7 +1011,7 @@
var Model = Backbone.Model.extend({
sync: function(method, model, options) {
setTimeout(function(){
- options.success();
+ options.success(null, 'success', {});
start();
}, 0);
}
Something went wrong with that request. Please try again.