Regression: Promises are rejected with non-Error objects

[adamreisnz]

SDK you're using (please complete the following information):

• 4.10.2

Describe the bug
Promises from the SDK are being rejected with non Error objects – response objects to be precise.
This is causing Bluebird to bark with the following message: Warning: a promise was rejected with a non-error
Version 3 of the SDK did not suffer this problem and was returning actual Error objects.

Can this be modified so that it the errors responses are wrapped in an actual Error object?
The response can still be passed as a property on that error object.

To Reproduce
Steps to reproduce the behavior:

1. Generate any error with the API
2. Catch the error
3. Observe that it's not an error but a response object

Expected behavior
For xero-node to throw Errors, not plain objects

[SerKnight]

Hi Adam - sorry on the delay here. I've been toying around with throwing a more distinct error object. We've also got a project in the works that's aim is to consolidate the (n) possibilities of error into a single, predictable path for the message.

Have a look at #463 if you haven't already.

Also curious if you can share a bit more of your expectation on this feature request.

Currently the SDK surfaces back the raw API response, which will be surfaced via the HTTP library request/response object.

Can you elaborate on what node framework you are using and what the error handling process is you go through now that is too cumbersome? That would be helpful to add that to the validation error object work #463 so we can improve everyone's error handling experience :)

Sample error request object

{
  response: IncomingMessage {
    headers: {
      server: 'nginx',
      'xero-correlation-id': '1eb59bc1-43a5-43af-baf7-976c6659fe2f',
      'x-appminlimit-remaining': '9990',
      'x-minlimit-remaining': '51',
      'x-daylimit-remaining': '4906',
    }
    statusCode: 404,
    statusMessage: 'Not Found',
    _consuming: false,
    _dumped: false,
    req: ClientRequest {
      _header: 'GET /api.xro/2.0/Invoices/not-an-id HTTP/1.1\r\n' +
        'user-agent: xero-node-4.11.2\r\n' +
        'xero-tenant-id: your-tenant-uuid' +
        'Authorization: Bearer your_access_token' +
        'host: api.xero.com\r\n' +
        'accept: application/json\r\n' +
        'Connection: close\r\n' +
        '\r\n',
      method: 'GET',
    },
    request: Request {
      href: 'https://api.xero.com/api.xro/2.0/Invoices/not-an-id',
    },
    body: "The resource you're looking for cannot be found"
  },
  body: Invoices { invoices: undefined }

We've also got some housekeeping to migrate the request library, away from request - that's just been a challenge as its the default option for the OpenAPI Spec generator we built this from.

[adamreisnz]

We are using the Bluebird Promise library, which is fairly strict in how it handles promise rejections.
It throws a warning if a Promise is rejected with a non Error object.

Basically, it is akin to having code like:

if (somethingIsWrong) {
  throw 'here is an error message';
}

This is generally considered bad practice, when errors are thrown you should always aim to throw actual error objects, as this also includes a stack trace etc:

if (somethingIsWrong) {
  throw new Error('here is an error message');
}

The same approach should be taken when rejecting promises or throwing errors from async functions which are processed as promises.

In this case, the Xero API request promise is rejected, but the object it is rejected with is not an error.

If you want to preserve the raw response object and make it accessible to the client consuming the API, may I suggest that you at least wrap this in an error, and pass it as a property on the error that can then be processed if needed, without triggering warnings about non-error objects being used?

For example, something like this:

const rawApiResponse = {...}; //How you obtain this is irrelevant

if (requestHasFailed(rawApiResponse)) { //Presumably you check for the status code of the response, e.g. 4xx, 5xx
  const error = new Error(`Xero API response error`);
  error.response = rawApiResponse;
  throw error; //Or: reject(error) if this was wrapped in a Promise
}

This way, you are conforming to best practices by throwing/rejecting with an actual error object, yet still making the raw API response available on the error object if needed for further processing.

We have a thorough error handling pipeline, which expects all errors to have the standard Error interface and properties, e.g. a name, a message, a stack trace etc. Having to intercept non Error objects and parse them first before we can process them is what's cumbersome.

Thanks.

[SerKnight]

Thanks Adam - this is really helpful and gives me a much clearer idea on how to potentially provide you a solution without a breaking change. Will put some work towards this as soon as we can.

[adamreisnz]

Awesome, thanks

…

On Thu, 29 Apr 2021, 04:58 Christopher Knight, ***@***.***> wrote: Thanks Adam - this is really helpful and gives me a much clearer idea on how to potentially provide you a solution without a breaking change. Will put some work towards this as soon as we can. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#500 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADXYQXIEAB6YPPKK7DUQLDTLA5ERANCNFSM4Y5NAPVQ> .

[stewartmcgown]

Missing any real error context makes development especially painful. Since I'm using higher-level error management, non-errors composed of complex nested objects (like the one you throw in this library) are impossible to parse.

[adamreisnz]

@SerKnight any progress on improved error handling here?