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

wrapError stripping ampersand-sync's err.message. #51

Open
EvanCarroll opened this issue Jun 9, 2015 · 3 comments
Open

wrapError stripping ampersand-sync's err.message. #51

EvanCarroll opened this issue Jun 9, 2015 · 3 comments

Comments

@EvanCarroll
Copy link

function wrapError is stripping ampersand-sync's err.msg. This is unfortunate as accessing it is often necessary. What about a patch to add the original contents of the err.message as options['ampersand-sync-error'] (setting that to arguments[2])?

@pgilad
Copy link
Member

pgilad commented Aug 26, 2015

This might be fixed by new &-sync version. Can you verify?

@EvanCarroll
Copy link
Author

I'm not sure how it could be.. We're calling options.error in ampersand-sync like this

options.error(resp, 'error', message);  // so the CB's third positional argument is *very* important.

Now, wrapError is defined like this...

var wrapError = function (model, options) {
    var error = options.error;
    options.error = function (resp) {
        if (error) error(model, resp, options);
        model.trigger('error', model, resp, options);
    };
};

But that's expecting a totally different signature. It's assuming our options.error = function (model, resp, options) {}

What if we want to get access to ampersand-sync's message?

{
  // this is how it would look if we were submitting options.error
  // straight to ampersand sync for use.. (no wrapError)
  // and we wanted to debug with options.message
  options: function (resp, errorOrSucess, messsage) {
    console.log("Error message is "+ message")
  }
}

We can't.. What we want is something like this...

var wrapError = function (model, options) {              
    var error = options.error;                           
    options.error = function (resp) {
        options['ampersand-sync-error'] = arguments[2];
        if (error) error(model, resp, options);
        model.trigger('error', model, resp, options);
    };
};

Which is what my local forked copy does... Or, change the signature.. We just barely document the error callback anyway..

save accepts success and error callbacks in the options hash, which will be passed the arguments (model, response, options). If a server-side validation fails, return a non-200 HTTP response code, along with an error response in text or JSON.

Our callback should either

  • Accept ampersand-sync's message as a positional argument
  • Stuff ampersand-sync's message into options or something so we can access it.

@pgilad
Copy link
Member

pgilad commented Aug 27, 2015

Thanks for the detailed response. I believe most things here are modeled after Backbone:
https://github.com/jashkenas/backbone/blob/master/backbone.js#L1884-L1890

Does resp not contain the error and/or the error message?
Would you consider submitting a failing test case PR for this?

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

2 participants