[discuss] change how errors are handled

[defunctzombie]

Update: This thread is now a discussion about whether to revert the behavior where all non-2xx responses are treated as errors. Please read the comments before just posting a +1 or -1 or other comment.

The arity change is not up for discussion and no one is disagreeing with that one.

---

Currently errors are handled in the following ways:

1. If the function to a .get or a .end as an arity of two, the first will be an error and the second a response. If it has an arity of one then only the response is provided.
2. In the case of non-200 responses, no error is emitted or set and the user must check res.status

I would like to discuss the following changes:

1. No more magic for the callback. Always require an error argument to make things simpler and more obvious at the callsites. The returned "request" object from .get or .end could still emit error to allow for some "centralize" handling.
2. All non-200 responses are treated as errors. This means that we detect the response status and make an error object with a .status field as needed. We do no other response processing like detecting an error message or whatever (leaving that up to the user). In the case of some network error or other xmlhttprequest error (the current error types) we don't set a .status since there wouldn't be one.

The above brings a few advantages in my mind (yes the changes would be major version bump breaking).

• less ambiguous function calls by removing magic arity use
• simpler error handling at the callsite since for ajax requests, any non 2xx is a non-success and therefore an error. Making the code to act on success simpler in the common case (currently one has to constantly check res.status in the callbacks

[gjohnson]

Yeah, I am kinda thinking we wrap up some outstanding bugs and tag 1.0 and then start 2.0 which would include things like this and other breaking changes.

[jamlen]

I'd like to see something like:

    request
      .post(config.endpoint)
      .set('Authorization', 'Bearer ' + token.secret)
      .send(json)
      .on(403, function(error){ self.emit('forbidden', error); }); 
      .on(404, function(error){ self.emit('notFound', error); }); 
      .on(418, function(){ makeABrew(); }); 
      .on(500, function(error){ self.emit('serverNotAvailable', error); }); 
      .on('error', function(error){ self.emit('dataTransmitError', error); }); 
      .end(function(res) {
         // ...
       });

Would this be feasible? Any comments?

[defunctzombie]

Feasible yes (it is just code). But I don't like it. The function(err, res){} syntax is nice and terse and you can expand on that do emit events into whatever else you want. It also creates a leaner testing surface for us since this module is not about many opinions but more fundamentals.

[jamlen]

Appreciated, but it does lead to a large if..else block, which I dislike more ;)

[defunctzombie]

@jamlen only if that is how you handle errors. Maybe a person's app handles them some other way :)

[jamlen]

@defunctzombie Fair point, I've only been using SuperAgent for about 3 weeks...and node for about 4 so I'm rather green! What is a better approach then?

[defunctzombie]

@jamlen depends what you are trying to accomplish :) Feel free to hop in the #express IRC channel on freenode and share what you are doing.

[gjohnson]

I am +1 on doing this now rather than later, more benefits by having this change than keeping what we have now.

[defunctzombie]

I thought about this more and see the following error setup.

• anything that is not a 2xx status will result in Error
• successful non 2xx responses will result in the error instance having a statusCode or status field
• unsuccessful responses (network issues, like the current behavior) will result in error instance with no statusCode or status field

The goal was to prevent the loss of information (network error or not) but maybe it doesn't really matter to an end user and the status could be set to some 5xx code for network errors.

[hallas]

What @defunctzombie said

[defunctzombie]

@gjohnson any more thoughts on this one? I am happy to make the code changes. I think they would be pretty small. It would be a breaking change.

[dcousens]

100% agree on this. That magic around the function arity is a real pain.

[defunctzombie]

This has been merged into master.

[kmalakoff]

Following up #554 and #580....

The new API is non-standard for Node.js callbacks. The request was successful so there should not be an error (eg. you can check the response status code). Only if the response cannot be returned should there be an error, eg. failed to parse the body.

[defunctzombie]

You could also make the claim that a non-successful http response is also
an error.

I am not married to the new api so could push out a revert it is really not
useful but there was not much pushback before and I provided lots of time.
Until something is released people don't try it.

On Wednesday, March 11, 2015, Kevin Malakoff notifications@github.com
wrote:

Following up #554 #554
and #580 #580....

The new API is non-standard for Node.js callbacks. The request was
successful so there should not be an error (eg. you can check the response
status code). Only if the response cannot be returned should there be an
error, eg. failed to parse the body.

—
Reply to this email directly or view it on GitHub
#283 (comment)
.

[kmalakoff]

Thank you for discussing this. I see your perspective about non-successful http responses.

My arguments would be:

1. if there is a response then I'm not convinced this is an error but a successful response with a non-success status. There's potentially data with it in addition to the status code that may be handled (for example, besides the status code, we also parse XML from S3 to check for request retry-ability: http://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html).

2. superagent provides a res.ok helper specifically for this purpose.

3. as a minor point around conventions/API design, I wouldn't expect a Node.js callback to return both an error and a value in the second parameter that needs to also be optionally checked. The new API is non-conventional and a bit messier now since it requires more validation logic to check the optional second parameter.

We would have to change 50+ calls because of this change to upgrade to 1.0.0 so let's get it right. What do other people think?

[basti1302]

This is a major breaking change, yet, as far as I can see there is no mention of it in the change log for 1.0.0 (https://github.com/visionmedia/superagent/blob/master/History.md is the changelog, or is there something else that I have overlooked?).

I think this change should be there in big bold letters. The way it is now, I had to find out about this change the hard way (my tests failed, debug them, find the piece of code that changed in superagent, check the git history of the lib/client.js until finally arriving at this issue).

PS: I really don't like the change, for my feeling, a successful HTTP request with a non-200 error code is fine in some situations. but that's certainly a matter of taste and I can see why people have different opinions.

PPS: That said, superagent is an awesome library, so thanks for this.

[defunctzombie]

Missing entry in changelog is an oversight on my part. I will add that.

As for the change, I can see where it is confusing because the method is
called 'end' and maybe making assumptions about the response is too high
level.

I will add tho that parsing errors can also have responses so the issue
with error having been only for network or when no response is not quite
accurate. To avoid passing two arts to the callback I can simply change to
err.res and remove res arg.

The idea behind this change was to make it easier to consume success
responses and ignore everything else. While it is true that a response is
still available for 4xx etc, I find that I always treat those completely
differently than success responses and this change was meant to make it
easier to avoid accidentally forgetting to check status. Whether this is
good or bad honestly depends on how high level you view this library. Yes,
the library makes requests, but so does node core. This library presents
some opinions about how to code up a request and consume it. Maybe this
change breaks the assumptions most people have about the library.

I will add that breaking existing code is in my mind insufficient to say
this change is "bad". I am aware it is a breaking change and knew it would
cause breakage. I even added a wiki page for migrating from 0.x to 1.x

Hearing more yay or nay here will help me identify of this should be
reverted. Obviously until released no one cared so happy to have the
discussion now.

On Thursday, March 12, 2015, Bastian Krol notifications@github.com wrote:

This is a major breaking change, yet, as far as I can see there is no
mention of it in the change log for 1.0.0 (
https://github.com/visionmedia/superagent/blob/master/History.md is the
changelog, or is there something else that I have overlooked?).

I think this change should be there in big bold letters. The way it is
now, I had to find out about this change the hard way (my tests failed,
debug them, find the piece of code that changed in superagent, check the
git history of the lib/client.js until finally arriving at this issue).

PS: I really don't like the change, for my feeling, a successful HTTP
request with a non-200 error code is fine in some situations. but that's
certainly a matter of taste and I can see why people have different
opinions.

PPS: That said, superagent is an awesome library, so thanks for this.

—
Reply to this email directly or view it on GitHub
#283 (comment)
.

[basti1302]

Wow, thanks for this detailed answer.

On second thought, what you say makes sense. I use superagent more like a tool doing the raw HTTP business for me. In that context, I personally would have preferred that superagent handles all HTTP status codes transparently and leaves me to interpret them. But superagent advertises itself as an Ajax library (Ajax with less suck, right?) and this gives things a slightly different perspective. Probably for most use cases this change is for the better.

I will add that breaking existing code is in my mind insufficient to say this change is "bad".

I never inteded to say that it's bad because it's a breaking change. I'm all for breaking changes. You upped the major version first digit, so that's totally fine.

Missing entry in changelog is an oversight on my part. I will add that.

I kind of did that already, see #584 . Feel free to ignore this and use your own wording if you prefer, though. Also, #583 seems to kind of duplicate this but also brings up a good point about updating http://visionmedia.github.io/

I even added a wiki page for migrating from 0.x to 1.x

Maybe adding a link to that wiki page directly to the Readme in a prominent place would be a good idea.

[kmalakoff]

I will add tho that parsing errors can also have responses so the issue with error having been only for network or when no response is not quite accurate.

This is a good subtle point to mention since there would be a response. In this case, I think a property on the error makes sense since there was not an error in the request, but an error processing the body of the response. Personally, I would treat this as a different case to a response with a non-successful status code, where I would still expect the arguments to be (null, res) since the request was successful even if the response status code indicates non-success, e.g. error are only for actual error control flow not non-success statuses.

[insin]

Errors returned as a result of a successful response's error status code don't have a .status as discussed above - is this a bug?

With 1.0.0, I have to check err and check if res is null or not when determining if I want to deal with client errors myself but pass off request errors or server error responses to an error handler.

[defunctzombie]

Yep, that is a bug. status should exist and so should err.res so that we
can follow node style callbacks as mentioned above.

On Friday, March 13, 2015, Jonny Buchanan notifications@github.com wrote:

Errors returned as a result of a successful response's HTTP status code
don't have a .status as discussed above - is this a bug?

With 1.0.0, I have to check 'err' and check if 'res' is null or not when
determining if I want to deal with client errors myself but pass off
request errors or server error responses to an error handler.

—
Reply to this email directly or view it on GitHub
#283 (comment)
.

[kmalakoff]

@landau obviously someone thought the same way in the past. Must have been someone with a Node background...maybe who maybe no longer likes callbacks or Node... 😅

[insin]

Having used 1.* for a bit, I think removing the pre-1.0 sugar which let you pass a callback to end() with only a res argument was worth making a breaking change for, for the sake of consistency with errback-style APIs..

Node-style callbacks and having the same API on both sides of the HTTP divide are what drew me to superagent in the first place over other libraries which use success/error style callbacks.

Forcing an (err, res) callback is consistent with that, but I think the change to populate err on 4xx and 5xx response codes introduces a new inconsistency - it makes err part of the normal flow for handling bad user input, which is a common scenario if you're doing things like navigation and form submission using superagent.

The pre-1.0 behaviour was clearly a design decision rather than an accident, since it was explicitly called out in the docs, and I now don't think it's worth breaking code where people were already using (err, res) callbacks and were expecting to handle a 4xx error by inspecting res.

[drypot]

agree on (err, res).

disagree 4xx err.
4xx is not err.
It's parts of meaningful formal procotol.
http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html

[defunctzombie]

@drypot at the danger of being too pedantic, it quite literally says

The 4xx class of status code is intended for cases in which the client seems to have erred

[marcello3d]

4xx and 5xx are totally errors. The majority of the time you don't want them.

[drypot]

@defunctzombie

Sorry for my poor english.
Ok. I think I have to start modifying thousands of my broken tests. :(
Next time it wil be good if there are more mild migration path.

[kmalakoff]

@drypot I wouldn't modify thousands of tests if I were you.

I'm happy to fork superagent to put back the old functionality. It there is enough interest, I'll even do it this weekend.

I just need a name...I'm thinking superagent-ls ...'ls' for less suck ;-)

[defunctzombie]

I have thought about this some more and I think the new behavior is better for the 90% case. If sometime later I feel that this is an incorrect choice or there are other maintainers with different opinions, we can revisit. I am not going to revert this in the near term.

I think this change, while controversial, is good. Maybe my thinking will change over time. Maybe this library is too popular and I should have not made any breaking changes. Well, in some ways that ship sailed. I asked for opinions on this early on, people weighed in and it seemed like a reasonable change.

[drypot]

@defunctzombie I can accept new style. Understood it. Style A, style b, It's not problem. but it's worring how many projects will be broken. Fortunatly I have tests.

[kmalakoff]

@defunctzombie totally disagreed, but it is your choice. The problem isn't that there is a breaking change, the problem is:

1. that the change breaks the Node callback conventions
2. is against the original design philosophy of superagent

Note that a 4xx or 5xx response with super agent is not considered an error by default. For example if you get a 500 or 403 response, this status information will be available via res.error, res.status and the others mentioned in "Response properties", however no Error object is passed for these responses. An error includes network failures, parsing errors, etcetera.

Forked: https://github.com/kmalakoff/superagent-ls and https://www.npmjs.com/package/superagent-ls. I'll aim for a first release the weekend.

I'm happy to accept for issues and pull requests for this fork.

Please star and use the superagent-ls repo to show your support for reverting the 1.x API 😏

[drypot]

Relaxed,
droped my mind,
some coffee,
modified 300+ test cases.

New style pros: in most cases simpler code.
cons: some side effects, some broken tests. for me 3% broken. not bad.

[kmalakoff]

@defunctzombie imagine the amount of work that has been created for the 3500+ users of superagent (not only developer time to update all calls and tests, but QA time to test our applications and libraries) and how many of us who do not agree with the change in the first place.

I think the better option is to revert the API change and introduce the success/error pattern.

I will spend my personal time this weekend updating superagent-ls and make a pull request to make the revert easier.

[kmalakoff]

superagent-ls has been released on npm: https://www.npmjs.com/package/superagent-ls and component.io

Please star and use the superagent-ls repo to show your support for reverting the 1.x API 😏

[aseemk]

I'm late to this party, but just want to say I'm impressed all-around by the thoughtful and reasonable discussion, and particularly @defunctzombie's open-mindedness. Great to see. =)

My 2¢:

I completely sympathize with @defunctzombie's logic. I myself have requested this change in behavior for request.js: request/request#606. There also, people were generally +1 to the idea, but the issue was ultimately closed with no PR to move forward with.

Indeed, in every place we use both request.js and Superagent, we have a helper function that does:

if (res.statusCode >= 400) {
    return callback(new Error(res.statusCode + ' response for ...'));
}

I majorly agree with @defunctzombie @marcello3d and others that this is the 90% common case, and it's much easier to work around and debug bugs with this than the opposite.

—

However, I also agree with @rauchg and others that there are legit cases of 4xx. The domain name lookup is a good example. Another is GitHub's API for querying stars/follows/etc.:

https://developer.github.com/v3/activity/starring/#check-if-you-are-starring-a-repository
https://developer.github.com/v3/users/followers/#check-if-you-are-following-a-user
https://developer.github.com/v3/gists/#check-if-a-gist-is-starred

All of those return empty 204 for "yes", or empty 404 for "no".

So it might be useful if this could be configurable. But maybe others have better ideas.

Either way, I'm overall +1 to this change, but understand others. Thanks @defunctzombie.

[aseemk]

One thing I'm not sure about is treating 3xx as errors. I understand browsers do this, but on Node, 3xx is a legit response more often than not, in my experience. E.g. 304 Not Modified.

Plus redirects obviously. Does Superagent still follow redirects, rather than treat e.g. 302 as an error?

[rauchg]

They simply are not errors. They're data. That's why you're always gonna have edge cases or debates about whether X status code was an error or not. The data got from end to end successfully. It was a success case, not an error case.

Think about it this way. The goal of the request function is to get the Response object, which has properties like status and text. Anything that gets in the way of producing that Response object is an error. Anything else is not.

That's why the signature is err, res. There are two possibilities: we got err, or we got res. We are proposing an in-between scenario where we got res, but it's also an err.

[aseemk]

That's a legit view @rauchg. I'd like to offer another view: my common use case for making requests is to talk to REST APIs. And with REST/HTTP, 4xx and 5xx are errors; 2xx and 3xx are not.

With this view, the goal of making requests is not to get a response so much as to get (or put or ...) a resource. And if the server responds with 4xx or 5xx, I didn't get the resource; I got an error.

However, my argument doesn't quite hold water with a library that indeed returns Responses as you mention. And I think this is what the debate boils down to: what layer this library lives at.

Perhaps the conclusion is that Superagent is at a lower transport level, not a higher application level.

[kmalakoff]

@aseemk I think you are right that Superagent is at a lower transport level, but unfortunately, there are legacy decisions that make it hard to use as a transport level. Superagent would really need a better API without globals and a more well-defined middleware API so that library writers could configure it without stomping all over one another.

Ideally, this sort of setting would be configurable in isolation so if I develop libraries that adhere to Node.js callback conventions, but other people are more familiar with browser interpretations of status codes we could do as we prefer.

[ggoodman]

I've tried to parse through all of this to figure out what the conclusion is.

From what I can see in practice: status >= 300 is now an 'error' and triggers that behaviour accordingly.

A major problem that I see with this approach and the Request#then adapter for Promise support is that all response metadata for non 2xx responses is lost.

What can we, as users of this lib, do to get around this?

[kmalakoff]

I'm still maintaining superagent-ls for the day sanity rules again 😉