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

Fix boom response error handling #164

Merged
merged 2 commits into from Mar 23, 2017
Merged

Fix boom response error handling #164

merged 2 commits into from Mar 23, 2017

Conversation

@kanongil
Copy link
Member

kanongil commented Mar 23, 2017

This fixes #82 without adding new options, while preserving most existing use-cases. This is done by only converting response errors to boom errors when the shortcut methods are used.

This also means that we can add payload information to the error, along with header information.

Finally, it ensures that clients can distinguish between response boom errors and native boom errors, which is impossible with the current implementation. If needed, this can be checked using the err.data.isResponseError property. Additionally, it is signalled as part of the message.

Note that this is a breaking change, and I have not updated the docs regarding the new err.data properties.

kanongil added 2 commits Mar 23, 2017
This exposes full details about the response, and enables clients to distinguish the errors from other boom errors
@geek geek self-assigned this Mar 23, 2017
@geek geek added this to the 12.0.0 milestone Mar 23, 2017
@geek

This comment has been minimized.

Copy link
Member

geek commented Mar 23, 2017

@kanongil thanks for the help. I'll update the readme. Do you have any other changes you'd like to see in wreck before v12 ships?

@geek geek merged commit 1544728 into hapijs:master Mar 23, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Mar 23, 2017

Thanks, I think I'm good.

Only thing is that it might be better to move the err.data.isResponseError property directly onto the boom object itself (and maybe name it isResponse). I'm not a fan of adding new properties to the boom object, but it makes it simpler to check for. As it is, you need to test it with something like this, since the data property might be undefined:

if (err.data && err.data.isResponseError) {
    /* do stuff */
}

vs

if (err.isResponse) {
    /* do stuff */
}
@geek geek mentioned this pull request Mar 24, 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.