-
Notifications
You must be signed in to change notification settings - Fork 2k
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
HAPI Integration Refactor #127
Conversation
…ollo-hapi # Conflicts: # src/integrations/hapiApollo.ts
…ollo-hapi # Conflicts: # src/integrations/hapiApollo.ts
@helfer The reason for a drop in code coverage is because I resolved one of the todo items which is to handle JSON parse errors in variables. As a result there isn't a test for that case. I have a set of changes that includes a test and fixes in all other integrations that increases the code coverage. Do you want me to include the changes in the PR or create a new one? |
Does |
@dahjelle no it doesn't. I just realized that my example in the readme is wrong. If you look at the integration test you can get an idea of how it is used but now options include { path, route, apolloOptions } where apolloOptions can still be a promise. I will fix the example now. |
I can confirm that it appears to address the issue in #123. Hooray! :-D
Good enough for me! |
|
||
function createErr(code: number, message: string) { | ||
const err = Boom.create(code); | ||
err.output.payload.message = message; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this? The err.output.payload.message
is derived from err.message
Boom.create(code, message)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so I struggled with this for a while. When I use Boom.create(500, 'Something went wrong') the payload returned simply says Internal server error
. I had to do this to change the message of the payload. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just double check and you are right. For an error 500, the message is overwritten.
See https://github.com/hapijs/boom/blob/master/lib/index.js#L136-L138
I like the use of the route prerequisites, it looks really clean. Still I have a question, why using |
@SimonDegraeve as I was using the all uppercase version I wondered which one was right. I agree I will lowercase hapi in my next commit. |
This is awesome! As far as I'm concerned, we can merge this as soon as we have a corresponding PR on the docs, which shouldn't take more than 10 minutes of work. 🎉 |
@SimonDegraeve @helfer I have decided to handle the renaming of Hapi #129 and code coverage in separate pull requests. There are a couple of other changes that are tagged for the 0.3 release that I will be working on today as well. I will update the docs once all the 0.3 are complete. |
This is a significant refactor of the HAPI integration to make the plugins more flexible and idiomatic. This should resolve the outstanding issues related to the HAPI integration. Specifically ( #97 and #123 )
WARNING: This refactor does result in breaking changes to the API.
TODO: