node-strava / node-strava-v3

API wrapper for Strava's v3 API, in Node
MIT License
356 stars 109 forks source link

wish: Only "2xx" and "3xx" responses from Strava are considered successful. #56

Closed markstos closed 5 years ago

markstos commented 7 years ago

As far as I'm aware, the only successful/expected responses from the Strava API are in the 200 range or the 300 range.

However, this module currently treats 4xx and 5xx responses as "successful", since the module largely passes through the response received from the REST calls. I confirmed that the request package doesn't even treat at 500 response as an error, since at the HTTP level, the HTTP request "succeeded":

var request = require('request');

request('http://httpstat.us/500', function (error, response, body) {
  console.log('error:', error); // Print the error if one occurred 
  console.log('statusCode:', response && response.statusCode); // Print the response status code if a response was received 
  console.log('body:', body); // Print the body
});

So currently with the node-strava-v3 module to do robust error checking, you have to first check that no err was returned from the request, then additional check that a 4xx or 5xx error was not returned.

Consider adopting the approach by the node-SES clients for the AWS email sending REST API:

https://github.com/aheckmann/node-ses/blob/ac619185d77604650987cfa0cb51c690f972ec43/lib/email.js#L218

It allows by for consolidated error handling by converting unexpected HTTP status codes into errors.

markstos commented 7 years ago

I've now worked on implementing, documenting and testing this in one of my branches: 251f9c42fc457c2ac93fd0b2ecd08a9b9138ed54

markstos commented 5 years ago

My fixes are being released in the next version. Closing this.