A callback is sometimes not called with an Error.

[DDR0]

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. https://github.com/DDR0/node-quickbooks/commit/d35551336621f37bfe62bbab45513f44e3f3ab42

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]

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.