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

A callback is sometimes not called with an Error. #68

Open
DDR0 opened this issue Mar 10, 2017 · 1 comment
Open

A callback is sometimes not called with an Error. #68

DDR0 opened this issue Mar 10, 2017 · 1 comment

Comments

@DDR0
Copy link

DDR0 commented Mar 10, 2017

A callback in the form of callback(err, results) should be called with an Error when being rejected.

For example, if a business logic fault occurs, the callback is returned with the error being set to the response body. The response body is not an Error, and has no stack trace. This makes it harder to track down business logic errors than it needs to be.

A possible solution might be to replace the module.request function return code, on line 1985, with

    if (callback) {
      if (err ||
          res.statusCode >= 300 ||
          (_.isObject(body) && body.Fault && body.Fault.Error && body.Fault.Error.length) ||
          (_.isString(body) && !_.isEmpty(body) && body.indexOf('<') === 0)) {
        var reportedError = err || body;
        if (!(reportedError instanceof Error) && body.Fault && body.Fault.Error && body.Fault.Error.length) {
          reportedError = new Error(body.Fault.Error.map(e=>`${e.code} ${e.element?`on ${e.element}: `:''}${e.Message} (${e.Detail})`).join())
          reportedError.name = body.Fault.type;
        }
        callback(reportedError, body)
      } else {
        callback(null, body)
      }

as I've done here. DDR0@d355513

I have not submitted a pull request because I don't feel my addition is up to snuff, code-quality-wise. For instance, template strings are not used anywhere else in the code, nor are arrow functions. However, I do feel the issue warranted a remark.

Thank you.

@chmac
Copy link

chmac commented Jun 6, 2017

I also discovered this issue trying to use bluebird's Promise.promisifyAll() to wrap the QuickBooks instance. I saw some weirdness and tracked it down to the lack of an Error being passed to the callback.

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