[discuss] change how errors are handled #283
Comments
|
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. |
|
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? |
|
Feasible yes (it is just code). But I don't like it. The |
|
Appreciated, but it does lead to a large |
|
@jamlen only if that is how you handle errors. Maybe a person's app handles them some other way :) |
|
@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? |
|
@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. |
|
I am +1 on doing this now rather than later, more benefits by having this change than keeping what we have now. |
|
I thought about this more and see the following error setup.
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. |
|
What @defunctzombie said |
|
@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. |
|
100% agree on this. That magic around the function arity is a real pain. |
Any completed response (non network error) also results in the error argument being provided to the end callback. This make it easier to handle successful responses versus failed responses. Along with the error argument, the response argument is also supplied. This behavior does not currently extend to `pipe()`, only to buffered responses that are parsed in full. see this issue for context: #283
Any completed response (non network error) also results in the error argument being provided to the end callback. This make it easier to handle successful responses versus failed responses. Along with the error argument, the response argument is also supplied. This behavior does not currently extend to `pipe()`, only to buffered responses that are parsed in full. see this issue for context: #283
Any completed response (non network error) also results in the error argument being provided to the end callback. This make it easier to handle successful responses versus failed responses. Along with the error argument, the response argument is also supplied. This behavior does not currently extend to `pipe()`, only to buffered responses that are parsed in full. see this issue for context: #283
Any completed response (non network error) also results in the error argument being provided to the end callback. This make it easier to handle successful responses versus failed responses. Along with the error argument, the response argument is also supplied. This behavior does not currently extend to `pipe()`, only to buffered responses that are parsed in full. see this issue for context: #283
|
This has been merged into master. |
|
You could also make the claim that a non-successful http response is also I am not married to the new api so could push out a revert it is really not On Wednesday, March 11, 2015, Kevin Malakoff notifications@github.com
|
|
Thank you for discussing this. I see your perspective about non-successful http responses. My arguments would be:
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? |
|
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. |
|
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 I will add tho that parsing errors can also have responses so the issue The idea behind this change was to make it easier to consume success I will add that breaking existing code is in my mind insufficient to say Hearing more yay or nay here will help me identify of this should be On Thursday, March 12, 2015, Bastian Krol notifications@github.com wrote:
|
|
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 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.
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/
Maybe adding a link to that wiki page directly to the Readme in a prominent place would be a good idea. |
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. |
|
Errors returned as a result of a successful response's error status code don't have a With 1.0.0, I have to check |
|
Yep, that is a bug. status should exist and so should err.res so that we On Friday, March 13, 2015, Jonny Buchanan notifications@github.com wrote:
|
|
@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... 😅 |
|
Having used 1.* for a bit, I think removing the pre-1.0 sugar which let you pass a callback to 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 Forcing an 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 |
|
agree on (err, res). disagree 4xx err. |
|
@drypot at the danger of being too pedantic, it quite literally says
|
|
4xx and 5xx are totally errors. The majority of the time you don't want them. |
|
Sorry for my poor english. |
|
@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 ;-) |
|
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. |
|
@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. |
|
@defunctzombie totally disagreed, but it is your choice. The problem isn't that there is a breaking change, the problem is:
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 😏 |
|
Relaxed, New style pros: in most cases simpler code. |
|
@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 I will spend my personal time this weekend updating superagent-ls and make a pull request to make the revert easier. |
|
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 😏 |
|
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 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. |
|
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? |
|
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 That's why the signature is |
|
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 Perhaps the conclusion is that Superagent is at a lower transport level, not a higher application level. |
|
@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. |
|
I've tried to parse through all of this to figure out what the conclusion is. From what I can see in practice: A major problem that I see with this approach and the What can we, as users of this lib, do to get around this? |
|
I'm still maintaining superagent-ls for the day sanity rules again 😉 |
Any completed response (non network error) also results in the error argument being provided to the end callback. This make it easier to handle successful responses versus failed responses. Along with the error argument, the response argument is also supplied. This behavior does not currently extend to `pipe()`, only to buffered responses that are parsed in full. see this issue for context: ladjs/superagent#283
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:
.getor a.endas 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.I would like to discuss the following changes:
.getor.endcould still emit error to allow for some "centralize" handling..statusfield 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.statussince there wouldn't be one.The above brings a few advantages in my mind (yes the changes would be major version bump breaking).
2xxis a non-success and therefore an error. Making the code to act on success simpler in the common case (currently one has to constantly checkres.statusin the callbacksThe text was updated successfully, but these errors were encountered: