Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return full response with the Boom HTTP error #173

Closed
dominykas opened this issue Apr 19, 2017 · 4 comments
Closed

Return full response with the Boom HTTP error #173

dominykas opened this issue Apr 19, 2017 · 4 comments
Assignees
Labels
Milestone

Comments

@dominykas
Copy link
Contributor

@dominykas dominykas commented Apr 19, 2017

With v12, the shortcut methods call back with a Boom error upon HTTP error. This makes it easier to handle HTTP errors when you want to treat them as errors, but it makes it trickier to handle them when you consider them a part of your daily life.

Would there be an objections, if the Boom error included the full response object here: https://github.com/hapijs/wreck/blob/master/lib/index.js#L525-L527?

This would allow code like this:

cb = (err, response, payload) => {

    if (err) {
       if (!err.data || !err.data.isResponseError) {
           return next(err);
       }
       response = err.data.response;
       payload = err.data.payload;
    }

    // proceed with checks for response.statusCode, etc
}

At the moment to handle the 2xx, 3xx and 4xx+ one needs to make checks twice - in the err handling code and in the rest of the code. I'm not certain if I'll go ahead with the pattern above, but regardless of that - having the full response, rather than just the headers might be useful. It would also expose the statusCode in a more reasonable location, which at the moment is only included as part of err.output (created by Boom?)

I'd probably also include statusCode in the "Response Error" message.

@geek geek added the request label Apr 19, 2017
@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Apr 19, 2017

@dominykas thoughts on including the response as the second parameter, which is what we do if there is an error reading the response.

@dominykas

This comment has been minimized.

Copy link
Contributor Author

@dominykas dominykas commented Apr 20, 2017

I don't mind - that might make sense, although I'm not sure about node conventions in such case? I think most of the time when the first param is the error, it's the only param?

If wreck already does that, though - I guess that's OK?

@dominykas

This comment has been minimized.

Copy link
Contributor Author

@dominykas dominykas commented Apr 20, 2017

Shall I PR that then?

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Apr 20, 2017

@dominykas yes please. Please go with your original proposal to include the response as a property

@geek geek closed this in #175 May 8, 2017
@geek geek added this to the 12.2.0 milestone May 8, 2017
@geek geek self-assigned this May 8, 2017
@geek geek added feature and removed request labels May 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.