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

Wrap error response with Boom #158

Merged
merged 2 commits into from Mar 21, 2017
Merged

Wrap error response with Boom #158

merged 2 commits into from Mar 21, 2017

Conversation

@geek
Copy link
Member

geek commented Mar 21, 2017

Fixes #82

@geek geek self-assigned this Mar 21, 2017
@geek geek added this to the 11.0.0 milestone Mar 21, 2017
@geek geek merged commit 5827bb8 into hapijs:master Mar 21, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@geek geek deleted the geek:82 branch Mar 21, 2017
@kanongil

This comment has been minimized.

Copy link
Member

kanongil commented Mar 22, 2017

Sorry, but this is a terrible implementation of the feature.

There are existing use-cases that requires information from the payload on status code errors, which is now impossible to retrieve. Further, all the helper methods don't even provide a way to access any headers on these errors.

As it is, something like h2o2 is not able to upgrade, and will need to find a replacement.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Mar 22, 2017

@kanongil I agree, would Boom.wrap not be better for this?

@geek

This comment has been minimized.

Copy link
Member Author

geek commented Mar 22, 2017

@kanongil and @AdriVanHoudt you have access to the full response object, which is passed as the second argument in the callback. Additionally, the response event provides the original request and response objects.

I am open to a better solution if either of you want to submit a PR.

@kanongil

This comment has been minimized.

Copy link
Member

kanongil commented Mar 22, 2017

Full access, yes – but it has been aborted. Which means I won't be able to get the payload.

@kanongil

This comment has been minimized.

Copy link
Member

kanongil commented Mar 22, 2017

As far as I can tell, this feature can only ever work for the helper methods, and the response behavior needs to be reverted.

@geek

This comment has been minimized.

Copy link
Member Author

geek commented Mar 22, 2017

I am open to a better solution if either of you want to submit a PR.

@kanongil

This comment has been minimized.

Copy link
Member

kanongil commented Mar 22, 2017

My PR would just be a revert of this commit. Then you can always create new option for this, as suggested in #82.

@geek

This comment has been minimized.

Copy link
Member Author

geek commented Mar 22, 2017

@kanongil doesn't this solve the issue? #163

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.