unhandled 'error' event in node

[jergason]

When making a request to a url that doesn't exist, axios crashes the server on an unhandled 'error' event.

var axios = require('axios');

axios.post('http://google.com').then(function(response) {
    console.log('woo a response', response);
  })
  .catch(function(response) {
    console.log('hrrm, an error', repsonse);
  });

// needed to prevent node from immediately exiting without actually making the request
setTimeout(function() {
  console.log('what in the world, why isn\'t this waiting to exit')
}, 5000);

It looks like the problem is no req.on('error') handler on https://github.com/mzabriskie/axios/blob/1d6430f667486ca9de390ccec242114b36c41377/lib/adapters/http.js#L80.

I'm not sure the best way to handle an error here though, since it doesn't fit the expected {data, status, headers, config} type of message the reject handler expects.

[mzabriskie]

@jergason good catch. I can add the error handler. Not sure how the error fits in the reject either.

A couple options:

1. Add an error property to the response object
2. Just provide the error object directly to reject

In either case you're going to have to check if an error exists:

// option 1
.catch(function (res) {
  if (!res.data && res.error) {
    console.log('error', res.error);
  } else {
    console.log('it worked!');
  }
});

// option 2
.catch(function (res) {
  if (res instanceof Error) {
    console.log('error', res);
  } else {
    console.log('it worked!');
  }
});

[mzabriskie]

I keep stewing on this, but still don't love either option, and haven't come up with anything else better. Stay tuned...

[kentcdodds]

Maybe I'm misunderstanding things, but if they're using .catch then the request didn't "work" and there shouldn't be data correct? I mean the response can come back with the reason for the error, but that could be considered the error right? So res will always be an error, right?

[ryanflorence]

I would expect catch to reject with an error object.

[jmdobry]

The issue is combing thrown errors and an http response into a single argument.

request.js example:

var request = require('request');
request('http://www.google.com', function (error, response, body) {
  if (!error && response.statusCode == 200) {
    console.log(body) // Print the google web page.
  }
})

In this example it's obvious which argument would be an Error object, caused by some exception in the code execution.

The response argument can be inspected separately, as it's status code might represent an error from a different domain, unrelated to our code's execution.

With axios:

.catch(function (res) {
  // what is res?
  // was there an exception? 
  // can I print a stack trace?
  // or do I just need to inspect the status code for 500, etc.?
});

The question is whether res will be the first or second (or both) argument from the first example. An Error object makes sense if an exception occurred, with a stack trace captured. But like @jergason said, it doesn't fit with {data, status, headers, config} which corresponds to the response argument in the first example.

A few options:

• Always pass an Error object and requiring the developer to know how to inspect it for exception/stacktrace vs response meta data
• Pass either an Error object or the response meta data, allowing the dev just check for which one
• (An extension of Abstract XHR to node's http #1) Create custom error class(es) (inheriting from Error) and expose them on axios' API. Example below:

.catch(function (err) {
  if (err instanceof axios.HttpError) {
    // determined by status code, no stack trace
    // handle non 2xx/3xx response
  } else if (err instanceof Error) {
    // caused by exception in code
    // handle error from code
  } else {
    // shouldn't get here?
  }
});

Another solution would be to follow how request.js does it (not how $http does it), as in don't decide for the developer that a particular http response should cause the promise to be rejected. In request.js, only exceptions in the code populate that error argument.

</spiel>

[kentcdodds]

Well explained @jmdobry... That last comment was interesting, but I think I'd recommend against going that direction for two reasons 1) you'd be forced to check the status code in your successful requests which would be annoying (especially in cases where you don't care if it was unsuccessful) 2) a lot of people use and love $http, so they'll expect axios to work similarly.

That second argument is a bit weak I think, but I thought I'd mention it anyway.

I think something like this would be reasonable:

.catch(function (result) {
  if (result.error) {
    // caused by exception in code
    // handle error from code
  } else {
    // determined by status code, no stack trace
    // handle non 2xx/3xx response
    console.log(result.data); // what came back from the server.
  }
});

[mzabriskie]

@jmdobry request's API doesn't apply to axios, since request is using a single callback to handle success/error. Versus using then/catch a la Promises. Also Promises limit then/catch to a single argument, so whether I like request's API or not, it can't be done the same way with axios.

I think I like just rejecting with the Error:

.catch(function (res) {
  if (res instanceof Error) {
    console.log(res.message);
  } else {
    console.log(res.status, res.data);
  }
});

[mzabriskie]

Actually it looks like a PR introduced this pattern already:

https://github.com/mzabriskie/axios/blob/master/lib/adapters/http.js#L35

Anyone have any heartburn over leaving it this way? I'd prefer not to change an API that people may already depend on in order to resolve this issue.

[kentcdodds]

I think that it's a totally reasonable/sensible solution 👍

[jmdobry]

@mzabriskie Yes of course. I was just using request's api as a contrasting example to clearly define this issue by showing what axios can't do. I should have been clearer in my comment.

[ryanflorence]

I'm not doing as much catching up as I should, but the value sent to reject should contain more than just an error (I wasn't very clear before).

Very often you'll send a non 2xx status code but still send data. You also may need the headers, etc.

I don't think you can just throw an error into the arg and call it good, there's a lot of information about the response you may need in the handler.

[kentcdodds]

I think that's what @mzabriskie is suggesting with:

.catch(function (res) {
  if (res instanceof Error) {
    console.log(res.message); // this was a js error
  } else {
    console.log(res.status, res.data); // this was a non 2xx or 3xx response status
  }
});

[ryanflorence]

seems fine to me then

[mzabriskie]

Yes @kentcdodds sums up the approach correctly.

.catch(function (res) {
  if (res instanceof Error) {
    // In this case a request was never sent to the server
    // Something happened in setting up the request that triggered an Error
  } else {
    // Here the request was made, but the server responded with a status code
    // that falls out of the range of 2xx
    // You will have the full response details available
    console.log(res.data); // The data that the server responded with
    console.log(res.headers); // The response headers from the server
    console.log(res.status); // The response status code
    console.log(res.config); // The config that was used to make the request
  }
});

[kentcdodds]

👏 totally reasonable api. Thanks again for making this thing dude.

[mzabriskie]

Thanks everyone for the dialog. I made the change as discussed and it is now on npm.