Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backbone.ajax wrapError() eats jQuery's 'textStatus' and 'errorThrown' arguments #3739

Closed
Cleod9 opened this issue Jul 31, 2015 · 4 comments
Closed

Comments

@Cleod9
Copy link

Cleod9 commented Jul 31, 2015

Please see:

options.error = function(resp) {

/* Line 1868 */
options.error = function(resp) {

The normal function signature for jQuery's ajax error() callback is the following:

function (jqXHR, textStatus, errorThrown)

But the second 2 params are not utilized in Backbone's option.error callback, which can result in missing error details. This can create "silent errors" when responses enter the error callback due to invalid JSON responses but still returned a 200 and status "OK".

Since Backbone's default implementation proxies this to jQuery, I think it's a good assumption to make that most users would have an interest in the textStatus and errorThrown arguments. Maybe it would be a good idea to pass these along with error(model, resp, options)? They could even be tacked onto the response object. It's likely many devs have had issues debugging ajax failures like this and just created catch-alls as a workaround, but I think by allowing easy access to these other two arguments it could allow developers to perform more explicit error handling.

(Somewhat) Relevant discussions:
#2162
#3520
http://stackoverflow.com/questions/16476874/catching-backbone-sync-errors

@jridgewell
Copy link
Collaborator

We pass them in the options argument.

@Cleod9
Copy link
Author

Cleod9 commented Jul 31, 2015

Thanks for the snappy response! We'll have to upgrade our version of Backbone in that case, not sure why I wasn't able to find those other issues during my initial search for answers...

@i-am-al-x
Copy link

i-am-al-x commented Jun 16, 2016

But at some version, you introduced the idea of actually executing "error" with these jQuery parameters. I am having real problems with this, to the extent I even wrote an "error()" function that attempts to guess which signature to use. Should it be the signature from "wrapError()" ...

        error(model,resp,options);

or should it be the signature from the end of "sync()" ...

        error(xhr,textStatus,errorThrown);

Pasted below please find the actual code from the end of "sync()". I took this from lines 1452-1458 of version 1.3.3. In my code base, I have v 1.2.3, and that version already had this code in it. Note well, that although you assign textStatus and errorThrown to "options", you also execute my "error" function (wrapped by wrapError()) with this signature. Why?

    // Pass along `textStatus` and `errorThrown` from jQuery.
    var error = options.error;
    options.error = function(xhr, textStatus, errorThrown) {
      options.textStatus = textStatus;
      options.errorThrown = errorThrown;
      if (error) error.call(options.context, xhr, textStatus, errorThrown);
    };

It seems to me that the introduction of the code to execute "error(xhr, textStatus,errorThrown) was a mistake, and should be reversed.
Or does somebody have an explanation for this?

Alexander

@i-am-al-x
Copy link

i-am-al-x commented Jun 16, 2016

Here is my workaround. It illustrates what the problem is.

  appUtils.errorMsgDuality = function(xhrOrModel,textStatusOrRespObj,errorThrownOrOptions){
    // New, jQ style signature: function(xhr   ,textStatus ,errorThrown)
    // Sometimes signature    : function(model ,resp       ,options)
    var errMsg = "";
    if('_changing' in xhrOrModel) {
        // This signature happened when I had server crash.
        var model   = xhrOrModel;
        var resp    = textStatusOrRespObj;
        var options = errorThrownOrOptions;
        if(resp.responseText)
          errMsg += resp.responseText;
    }else {
        // This is the signature I read somewhere.
        var xhr         = xhrOrModel;
        var textStatus  = textStatusOrRespObj;
        var errorThrown = errorThrownOrOptions;
        if(typeof textStatus === 'string')
          errMsg += textStatus;
    }
    return errMsg;
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants