Nexmo / nexmo-cli

Nexmo CLI (Command Line Interface)
https://nexmo.com
MIT License
78 stars 52 forks source link

Update to use nexmo-node v2.0.0 and check error callbacks #123

Closed leggetter closed 7 years ago

leggetter commented 7 years ago

The Nexmo Node library v2.0.0 has been updated to provide more information in the error callback. It is now in the format:

{
  statusCode: STATUS_CODE,
  body: JSON_BODY,
  headers: HEADERS
}

Any existing checks should be changed from directly looking at properties on the error object to checking error.body.{property}.

The addition of the statusCode property may also be useful in better determining the cause of the errors.

If the updates in v2.0.0 don't help https://github.com/Nexmo/nexmo-node/issues/78 then additional comments would be welcome in order to provide more useful error callbacks.

cbetta commented 7 years ago

Hey @leggetter, it's a good step but I'm not sure it's making things much better yet for the CLI.

For example, this is my current check:

  response(error, response) {
    this.debug(error, response);

    if (error && error.message) {
      this.emitter.error(error.message);
    } else if (error && error['error-code']) {
      this.emitter.error(error['error-code-label']);
    } else if (response['error-code'] && response['error-code'] !== '200') {
      this.emitter.error(response['error-code-label']);
    } else if (response['status'] && response['status'] !== '0') {
      this.emitter.error(response['status_message']);
    }
  }

So let's break this down:

Check for an error.message

This is still in place I think when the server throws a 500.

Check for an error[error-code]

This is still in place but has now moved to error.body[error-code]. This body response sometimes has an error-code that signifies an error, but it also appears in successful responses! See the next issue.

Check for an response['error-code'] != 200

This is basically when the Node library returns a success status header, but still has an error in the body! This hasn't changed as far as I can see.

Check for response['status'] !== '0'

Finally, sometimes the API throws a response[status], seemingly not throwing an error, but then still signifying an unsuccessful API call as the status will be non-zero. This hasn't changed as far as I can see.


Proposed solution

As fixing the API will take a while longer I propose the following change: move my nested if-else statement into the Node library, clearly preparing an error and a response object. Ideally, the response should be just the API body JSON.

The error object should contain the data we really need as users of the library:

We could apply the same to the successful response, but I'm not sure this is needed.

cbetta commented 7 years ago

With this in place, my code could just change to:

response(error, response) {
    this.debug(error, response);

    if (error) {
      this.emitter.error(error.message);
    }
  }

Now this is a bit of a weird use case, but here is a more realistic example:

nexmo.number.cancel('GB','44555666777',function(error, response) {
   if (error) { 
     // 100% sure an error occured here
   } else {
     // 100% no error occured here
   }
});

Currently the error/success switch is a lot more iffy.

This would ideally also make eventual Promises a lot easier:

nexmo.number.cancel('GB','44555666777')
  .then((response) => {
     // 100% no error occured here
  }).catch((error) => {
     // 100% sure an error occured here
  });
});