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

Added a method to extend output error #47

Closed
wants to merge 7 commits into from

Conversation

imayolas
Copy link

Use case: To take advantage of Boom and at the same time send custom data to the output object, you must manually attach properties to the the_output.payload_ boom instance. The following code is more elegant and chainable: Boom.forbidden('My message').extendOutput({customError: 123});

@hueniverse
Copy link
Contributor

I'm not sure how useful this is, but either way, there must be a nicer function name...

@imayolas
Copy link
Author

I think it's useful in some contexts for an API to return errors with additional information, like an internal error code. Let's say for example that a database finds a duplicate username while creating one, and the ORM throws an error. You might want the API to return en error , with an internal èrrorCode property, to make it easier to identify that specific error in the frontend. I particularly don't like displaying the API error messages to the end user, especially as it makes localization of frontends more complex.
I am going to use it anyway for a project, and felt it was worth the addition. Feel free to decline it if you don't see the use.
As for the name & completing the code coverage to 100%, I'd appreciate if you give me a hand here.

@hueniverse
Copy link
Contributor

I'll let @arb decide.

@@ -45,6 +45,9 @@ internals.initialize = function (error, statusCode, message) {
error.reformat = internals.reformat;
error.reformat();

error.extendOutput = internals.extendOutput;
error.extendOutput();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you calling it here? Doesn't look like it's doing anything.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true. useless.

@arb
Copy link
Contributor

arb commented Jan 26, 2015

Few other things

  • I'm not sure of the use case on this as well. You have to create the error object first and then extend the properties anyway, what is adding this function really saving you?
  • Please rebase to fewer commits
  • Tests are not passing because you do not have 100% code coverage.

@arb arb added the feature New functionality or improvement label Jan 26, 2015
@arb arb self-assigned this Jan 26, 2015
@imayolas
Copy link
Author

I'm not sure of the use case on this as well. You have to create the error object first and then extend the properties anyway, what is adding this function really saving you?

I think it comes down to flavour. I personally think it's more way verbose this code:

var boom = Boom.forbidden( "Username already exists" );
boom.output.payload.errorCode = "duplicatedUsername";
reply( boom );

than this one:

reply( Boom.forbidden("Username already exists.").extendOutput({errorCode: "duplicatedUsername"}) );

Now, as a second thought, the main use case here is, like in the example, to add an internal errorCode. Perhaps the balance between simplicity and expressivity here is to rename the method to .code, which expects a string or an integer. thoughts?

Just openly tell me if u think it's worth adding this method or not, and I'll fix the rest. A no go here is understandable.

@imayolas
Copy link
Author

@arb Comments?

@arb
Copy link
Contributor

arb commented Jan 30, 2015

I think we'll pass on this right now. We can re-examine this if more people ask for it. Doesn't seem useful enough to justify changing the API.

Thanks for contributing. Lots of other things you can do in the hapijs world if you're looking for more open source work 😄

@arb arb closed this Jan 30, 2015
@imayolas
Copy link
Author

Thanks @arb. Fully understand. We're building our first project with hapijs and we're very very happy with it. We'll be happy to contribute where we humbly can.

@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New functionality or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants