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 errors #7

Merged
merged 3 commits into from May 29, 2018
Merged

Fix errors #7

merged 3 commits into from May 29, 2018

Conversation

flintinatux
Copy link
Contributor

dilbert errors

In the past, sox has been less than helpful auto-wrapping vanilla errors with Boom representations. They all just ended up as 500's, and the original message was lost in time and space.

Now we can surface those custom error messages. You'll still get a 500, but the message property should be helpful.

to test:

  • Read the code.
  • Wait for the green.
  • Find a squirrel in sox.

Copy link
Member

@mgreystone mgreystone left a comment

Choose a reason for hiding this comment

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

Code looks good, but i'm confused. I assumed we purposefully swallowed error details just like hapi & paperplane?

@@ -1,10 +1,8 @@
language: node_js
node_js:
- '8'
- '7'
- '6'
Copy link
Member

Choose a reason for hiding this comment

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

Are we locking sox to node 8 just for boom?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I double-checked our repos, and we don't use sox in anything less than node 8, so I figured it was safe.

Copy link
Contributor

@spencerfdavis spencerfdavis left a comment

Choose a reason for hiding this comment

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

💯

@flintinatux
Copy link
Contributor Author

@mgreystone, looking back at the client-side portion of sox after our talk earlier, I'm seeing this in client/toError.js:

const toError = ({ data, message, name, status }) => {
  const err = new Error(message)
  err.data = data
  err.name = name
  err.status = status
  return err
}

We already treat message and name coming in from the server as though they are different values. In Boom they are always the same, but that's irrelevant. I'm thinking that having a message string that's helpful isn't a serious bleed of information, and that passing structured data along with it the way you did in the past is also good. And if it's something we want to backport to paperplane so we're consistent, then that's good too.

@spencerfdavis
Copy link
Contributor

@spencerfdavis spencerfdavis merged commit 6994c61 into master May 29, 2018
@spencerfdavis spencerfdavis deleted the fix-errors branch May 29, 2018 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants