Conversation
Marsup
left a comment
There was a problem hiding this comment.
I stopped reviewing when I saw travis actually tested against hapi v16. I can resume later :)
| node_js: | ||
| - "4" | ||
| - "6" | ||
| - "8" |
There was a problem hiding this comment.
You're missing "9" and "node". See hapi for that one.
package-lock.json
Outdated
| @@ -0,0 +1,2125 @@ | |||
| { | |||
There was a problem hiding this comment.
We don't use package locks, just add it to the .gitignore.
| "joi": "13.x.x" | ||
| }, | ||
| "peerDependencies": { | ||
| "hapi": ">=11.x.x" |
There was a problem hiding this comment.
You'll probably want to bump that.
| const describe = lab.describe; | ||
| const it = lab.it; | ||
| const expect = Code.expect; | ||
| const { describe, it } = exports.lab = Lab.script(); |
There was a problem hiding this comment.
You can also get the expect in this destructuring.
There was a problem hiding this comment.
Wouldn't it then get expect from lab instead of getting it from Code?
| return reply(request.params.user); | ||
| } | ||
| request.cookieAuth.set({ user: request.params.user }); | ||
| return reply(request.params.user); |
There was a problem hiding this comment.
reply is not a v17 thing anymore, I'm surprised it would work.
There was a problem hiding this comment.
He said it's not intended to be released
| handler: function (request, reply) { | ||
| server.route({ | ||
| method: 'GET', path: '/login/{user}', | ||
| config: { |
There was a problem hiding this comment.
config is now options if I'm not mistaken.
|
Sorry, conf reviews are the worst, I didn't see your 2 steps thing, you can disregard some of the comments :p |
|
@Marsup No problem! I'll ignore the package-lock file and fix travis conf anyway. (as per the lock file, I just committed it because it exists in hapi repo - as npm-shrinkwrap.json file - but I'll remove it) As soon as this lands I'll start working on the actual update to support hapi v17. |
|
@vinicius0026 Just so happens I worked on this today. ;) Welcome all comments on this #174. |
|
@mrlannigan It's unfortunate that we both worked on the same thing at the same time. But since your PR is a complete update to hapi v17 (as mine is still a WIP), I'll gladly close this once yours lands. |
|
@vinicius0026 Yea, my apologies for not posting it. |
|
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. |
As previously stated in #172, this is the first step to provide support to hapi v17.
All dependencies have been upgraded to latest version, except for hapi, that was upgraded to v16.
Tests were fixed to adhere to new syntax as per lab@15, but no changes were made to the actual code.
Support for node < 8 was dropped, since
async awaitsyntax is used in tests.This is not intended to be released, but serves as an intermediary step since this is my first contribution to hapi ecosystem.
All comments are welcome.