Superagent: [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 https://github.com/visionmedia/superagent/pull/554 and https://github.com/visionmedia/superagent/issues/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 [email protected]
wrote:

Following up #554 https://github.com/visionmedia/superagent/pull/554
and #580 https://github.com/visionmedia/superagent/issues/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
https://github.com/visionmedia/superagent/issues/283#issuecomment-78401506
.

[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 [email protected] 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
https://github.com/visionmedia/superagent/issues/283#issuecomment-78533352
.

[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 [email protected] 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
https://github.com/visionmedia/superagent/issues/283#issuecomment-79033090
.

[defunctzombie]

@kmalakoff to follow nodejs style callbacks, I can simply remove the response object from the callback when there is an error. I have already added the err.response field and err.status fields so the extra response arg would not be needed.

Obviously there is still the larger opinionated API concern, but as far as callback style, that can easily be resolved.

[mdebruijne]

Hi @defunctzombie,

I was also bitten by this API change without a warning in the release notes. These things can happen and your work on SuperAgent is really appreciated!

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.

My two cents;

• change to mandatory error object in the callback; with a big warning of this API change in the release notes it is easy to sed over codebase to add the error object to the callback. If it makes your life easier then go for it. ;-)
• change to treat non 2xx codes as errors; this change doesn't make sense to me, because it is very uncommon for a library to generate an error object based on the content outside of its own scope. Personally I don't know another library that behaves this way.

Analogies to different libraries;

• DNS library generates an error on a successful DNS A-record query, because it doesn't like the returned IP address
• cookie-parser generates an error, because it doesn't like the cookie content
• mongoose generates an error, because it doesn't like the contents of a document

To me, these analogies are the same as a "raw" HTTP library that generates an error, because it doesn't like the content (response) on a successful HTTP request.

The library does a request and can receive a response, doesn't it make sense to put the response in the response object instead of putting the response in the error object? Only if it doesn't receive a response then return an error object.

Also, non 2xx codes like, 301/302, 401 and even 404 can be desirable/mandatory in a lot of situations;
301/302; test the redirect from http to https
401; test if data is not accessible without authentication
404; test if data is not accessible outside of the organisation network

If any of these tests return a 2xx code then security is broken in the application or infrastructure, but now the SuperAgent HTTP library considers everything is working as expected. If the application is not broken and all security rules function as expected the SuperAgent HTTP library now considers this an error.

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

Is it possible to add the link to this wiki page in the release notes? I was not able to find it.

Again, many thanks for your work on SuperAgent!

[basti1302]

change to treat non 2xx codes as errors; this change doesn't make sense to me, because it is very uncommon for a library [...] Personally I don't know another library that behaves this way.

Maybe it's better to compare this with how other http libs handle this cases than comparing it to DNS or cookie parsing libs.

• jQuery Ajax distinguishes between success and error callbacks. The success callback is only called for http status 200-299, other http statuses end up in the error callback. So that's very similar to [email protected]'s behaviour (also technically the callback style is different, judging what is an error and what not is the same).
• AngularJS' $http: https://docs.angularjs.org/api/ng/service/$http also uses success and error callbacks. Same as jQuery, same as [email protected].
• request/request: Handles http status codes like [email protected], that is, no http status codes creates an error.

So obviously both ways are well known patterns. Actually, I think superagent is closer to jQuery.ajax and AngularJS' $http than to request/request.

Also, all your examples of use cases for 3xx/4xx status codes are about tests...

[mdebruijne]

Thanks for sharing your valuable insights, it is really appreciated.

I guess the different expectations are based on comparing SuperAgent with backend/testing libraries versus comparing SuperAgent with frontend libraries.

If you look at SuperAgent from a frontend perspective then it makes more sense to make it behave like other frontend libraries like AngularJS and jQuery. You still need to look very closely to the details. For example AngularJS doesn't use the error object with redirects, but SuperAgent does.

In the end, it really doesn't matter that much as long as (changes to) the behavior is well documented.

For the people who use SuperAgent as a backend/testing library;
after a sed from .end(function (res) to .end(function (err, res) all tests work as expected with SuperAgent version 1.1.0 (we test for almost all HTTP scenarios/return codes).

[nw]

FWIW.

Dojo, Prototype, Mootools, YUI (io) all follow the same pattern as well.
Treating anything above 2xx as an error.

Things I found interesting:

• Prototype allows one to register on2xx, on3xx, on4xx and on5xx
  listeners that when set override the simplified success and error
  listeners.
• Mootools allows passing your own custom isSuccessful function to
  override behavior.

On Fri, Mar 13, 2015 at 4:52 PM, mdebruijne [email protected]
wrote:

Thanks for sharing your valuable insights, it is really appreciated.

I guess the different expectations are based on comparing SuperAgent with
backend/testing libraries versus comparing SuperAgent with frontend
libraries.

If you look at SuperAgent from a frontend perspective then it makes more
sense to make it behave like other frontend libraries like AngularJS and
jQuery. You still need to look very closely to the details. For example
AngularJS doesn't use the error object with redirects, but SuperAgent does.

In the end, it really doesn't matter that much as long as (changes to) the
behavior is well documented.

For the people who use SuperAgent as a backend/testing library;
after a sed from .end(function (res) to .end(function (err, res) all
tests work as expected (we test for almost all HTTP scenarios/return codes).

—
Reply to this email directly or view it on GitHub
https://github.com/visionmedia/superagent/issues/283#issuecomment-79514517
.

[kmalakoff]

@defunctzombie understood. I'm not sure if the subtlety was clear.

What my recommended approach is that only in the case of there being an actual error would the response be put on the error. So for all successful requests regardless of status code: (null, response) and for errors (_.extend(err, {response})).

Basically, I agree with the single parameter principle for Node-style callbacks and prefer error argument only when there is an actual error like a parsing error.

[kmalakoff]

@basti1302 I think your argument is a false equivalence argument using an old-fashioned/jQuery-like success/error pattern.

We use Node-style callback as our preferred pattern for browser and server code, and we follow that convention strictly. This is what partially drew us to superagent in the first place...a cross-platform API based on Node.js conventions.

If superagent provides a node style callback pattern, it should follow the Node convention. If superagent provided an old-fashioned/jQuery-like success/error API, it would make sense to follow those conventions. Same if it provided a promises-based API, it should follow those conventions.

[dcousens]

Although I agree with @kmalakoff that only in the event of an actual exception should the err argument be not null, I can't help but recognize ow much I personally rely on an error for things like a 404 intuitively which, obviously, is a valid response.

That said, I guess I'd just quickly adjust to that and enforce the expected status code everywhere.

[kmalakoff]

@dcousens it's funny, but I feel exactly the opposite. I totally see that people coming from a jQuery background would have different expectations since they are using a different convention.

I bet it is that when I found superagent, the Node convention made much more sense to me than the way jQuery did this determination for you in a forced branching code flow between success/error. If I HEAD an endpoint with the purpose to check if a resource exists, I check the response's status code for a 404. I doesn't feel like the request failed but just a successful request/response cycle to check if a resource exists so (null, res) feels correct. It is subjective on whether the 404 is an error or an expected response status code so it feels like it is the application's responsibility to classify not the library's. An error in Node-callback conventions is a very serious proposition....

In https://github.com/visionmedia/superagent/pull/554, @rauchg wrote:

It's a really bad idea to consider HTTP-level error codes actual errors. Maybe a better way to think about it is, is a stack trace helpful at all for a 404? That's certainly not an Error.

I wish the 1.0.0 change wasn't made in the first place because it makes it harder to switch back if people are now in the process of changing their code. We're still locked at 0.x awaiting the final call.

@defunctzombie are you staying the course, reverting, needing more input, or still considering?

[defunctzombie]

@kmalakoff

I don't know if I am sold yet. I don't 100% agree that all errors are things that need a stacktrace. It is absolutely valid to produce an error for business logic. Even things like the parsing error produce meaningless stack traces because the actual error is that the response body could not be parsed but the stacktrace is where the parsing tried to happen. Here the server sent back a response, just not a valid/parsable one and we error. Technically in that case we too should not have the error object because the server sent you a response?

I can simply remove the second response arg and leave error.response and now are are following nodejs style conventions so that argument doesn't sway me.

The only argument that might sway me is that this library is meant to be lower level and not make such a judgement call about the "success" of a response in the end callback. But honestly, I am not 100% sure that is the case.

It is subjective on whether the 404 is an error or an expected response status code so it feels like it is the application's responsibility to classify not the library's. An error in Node-callback conventions is a very serious proposition

I totally understand this point and agree, however I also think that it is not wrong to check the error for codes and other things if you want to take certain actions on said errors. I realize that handing this now is different code than before but that is why the change is a breaking one.

Looking at the status codes page here

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

In the context of http responses and higher level libraries, it would not be that crazy to consider non 2xx (success) responses to be errors.

Maybe people don't like this assumption for any level of http request library and that is the core issue, but I have yet to hear enough votes to really make that call right now.

@mdebruijne

cookie-parser generates an error, because it doesn't like the cookie content

superagent parser will generate an error if it cannot parse the response body, yet there is a response

[rauchg]

Errors are a problem in the logic. An unexpected outcome. A 404 can be perfectly expected. The example I usually give is:

https://twitter.com/rauchg/status/556153308168323072

[kmalakoff]

@defunctzombie I'll explain a little further using @rauchg's unexpected outcome refinement......

Here the server sent back a response, just not a valid/parsable one and we error. Technically in that case we too should not have the error object because the server sent you a response?

If you receive a response, you should use the (null, res) signature because you expect a response to the request regardless of the status code. But because superagent has an helper API layer for parsing those response bodies, you also expect a parsable response but if you did not get one, it is an error and so in this case returning an error makes total sense (eg. an actual error due to malformed JSON, XML, cookies, etc) so the (_.extend(err, {response})) signature is right in this case because it is an actual error with the response because it is invalid.

The first case is expected whereas the second case is unexpected and would require investigation (eg. stack trace, looking into the parser, looking into the server that sent the response).

I think using this sort of rationale keeps the responsibility of the two superagent API layers much cleaner and more intuitive. I do agree with and like the error with a response property solution for actual errors.

Is this more clear in explaining the differences on when a user of superagent would expect to receive a response vs an error signature?

[kmalakoff]

@defunctzombie just wondering if you have an update on this? It is totally fine if you disagree or need more input.

I just want to know so I can decide about forking superagent or not to get the benefits of the latest fixes and features.

[defunctzombie]

@kmalakoff I really wanted more people to weigh in on it :/

If I HEAD an endpoint with the purpose to check if a resource exists, I check the response's status code for a 404. I doesn't feel like the request failed but just a successful request/response cycle to check if a resource exists so (null, res) feels correct

I am learning towards undoing the change as I think the confusion lies with using end() and having pre-existing expectations around that always returning a response if one was available. The other thing that pushes me towards undoing this change is all of the existing superagent stuff that expects err, res and is unlikely to all be updated appropriately.

I will add tho that the cases like you describe I find far fewer instances of than the case of having to write a check for response status 2xx in handlers before trying to use the response.

[marcello3d]

What's easier to debug?

1. You're expecting a normal response object when you get a 4xx response, but instead you get an Error('HTTP Status Error 400 Bad Request') w/ stack trace
2. You're get a 4xx response with no error which causes a ripple effect elsewhere in the app. (Maybe you're posting to an API, get a 403, but the app carries on because you forgot to check the status code)
   
   
   
   • And if you forget to check the error, hopefully the undefined 'response' will trip you up
   
   

My feeling is more time would be wasted on the latter scenario than the former.

[rase-]

It could be useful to introduce another API for this and revert the current behaviour of .end().

We could have something like a .success() and .error() handler that are mutually exclusive (in a way) with .end(), where any response that qualifies as a valid response currently would end up in .success(), and any errors plus error status codes would end up in the .error() callback.

Discussed this briefly with @defunctzombie and @rauchg (ping for some more thoughts/discussion).

[landau]

Docs appear to be out of date. http://visionmedia.github.io/superagent/#error-handling

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.

When an HTTP error occurs (4xx or 5xx response) the res.error property is an Error object, this allows you to perform checks such as:

[kmalakoff]

We could have something like a .success() and .error() handler that are mutually exclusive (in a way) with .end(), where any response that qualifies as a valid response currently would end up in .success(), and any errors plus error status codes would end up in the .error() callback.

@rase- I really like that solution. :+1:

It's in alignment with what I wrote before about expectations against conventions:

If superagent provides a node style callback pattern, it should follow the Node convention. If superagent provided an old-fashioned/jQuery-like success/error API, it would make sense to follow those conventions. Same if it provided a promises-based API, it should follow those conventions.

We don't use anything but the .end (err, res) signature since we use Node's callback pattern across all of our client and server code. We never use .on 'error'or the .end (res) signature. We'll never use the .success/.error signature, but those people who come from a jQuery background would find it the most familiar and in alignment with their expectations.

[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... :sweat_smile:

[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 :smirk:

[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 :smirk:

[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: https://github.com/request/request/issues/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 :wink: