hapi-auth-udaru package #505
Conversation
.travis.yml
Outdated
| @@ -1,8 +1,7 @@ | |||
| language: node_js | |||
|
|
|||
| node_js: | |||
| - '6' | |||
| - '7' | |||
| - '8.9' | |||
There was a problem hiding this comment.
This is definitely not correct. We should test the Hapi v17 bits only on 8 and 9, but the rest also on Node 6.
We can drop 7.
There was a problem hiding this comment.
I agreed. But I'm not sure on how to configure Travis for this. Any hint?
There was a problem hiding this comment.
run the test everywhere, but then the projects that are 8+ should not run their tests if node < 8.
There was a problem hiding this comment.
Ok, good idea. Will do it.
packages/hapi-auth-udaru/README.md
Outdated
|
|
||
| ... | ||
|
|
||
| const server = new Hapi.server() |
There was a problem hiding this comment.
No. Dumb me. Good catch.
| @@ -0,0 +1,65 @@ | |||
| 'use strict' | |||
|
|
|||
There was a problem hiding this comment.
I think we should move the benchmarks into their own shared package. Otherwise they are just duplicated.
There was a problem hiding this comment.
Good point. Especially since I'm working on the fastify version. Will do it.
There was a problem hiding this comment.
Actually I was thinking...if these benchmark are run from within the entire udaru repository, why do we even need a shared package at all? Couldn't just move it to the root folder and that's it?
There was a problem hiding this comment.
we'll likely have to support both plugins for some time, anything that cuts down on duplication would be welcomed
ShogunPanda
force-pushed
the
hapi-auth-udaru
branch
from
56589a5
to
4e51963
Compare
|
Note: this is currently base after #508. Make sure to wait for it to be merged, then rebase after master before merging. |
ShogunPanda
force-pushed
the
hapi-auth-udaru
branch
from
4e51963
to
84a35bc
Compare
|
@mcollina Due to force push, I can't see your previous Travis commit. But I can confirm is now run on 6 and 8.9 |
|
@mcollina The new name for the new package is because udaru-hapi-* will be for v16-, while hapi-auth-udaru (which contains both) is for v17+. I picked up a new name trying to be consistent with other plugins in the Hapi plugins list. |
ShogunPanda
force-pushed
the
hapi-auth-udaru
branch
from
dfd8038
to
9b81a3b
Compare
| ``` | ||
|
|
||
| ## Usage | ||
|
|
||
| ```js | ||
| const Hapi = require('hapi') | ||
| const UdaruPlugin = require('@nearform/udaru-hapi-plugin') | ||
| const UdaruPlugin = require('@nearform/hapi-auth-udaru-16') |
There was a problem hiding this comment.
with this naming convention are we causing confusion, could someone think it is udaru-16 and not to do with hapi-16?
would it be better to have udaru-auth-hapi-16 & udaru-auth-hapi-17 for example and any other server support use the same convention udaru-auth-fastify etc.
There was a problem hiding this comment.
You're probably right.
The reason I don't want to use 17 in the newer package is because if in the future hapi goes 18 and we're still compatible we probably don't want to rename the package.
There was a problem hiding this comment.
yes I much preferred the old naming convention, I know it's not hapi convention but I don't care. Am leaning back towards:
- udaru-core
- udaru-hapi-16-plugin
- udaru-hapi-plugin
- udaru-hapi-server
Thoughts?
There was a problem hiding this comment.
yes sounds good to me.
.travis.yml
Outdated
| @@ -2,7 +2,7 @@ language: node_js | |||
|
|
|||
| node_js: | |||
| - '6' | |||
| - '7' | |||
| - '8.9' | |||
There was a problem hiding this comment.
this should be '8', and we should also add '9'.
There was a problem hiding this comment.
Still the same here, did you push?
package.json
Outdated
| @@ -42,7 +44,8 @@ | |||
| "lint": "standard", | |||
| "postinstall": "lerna bootstrap", | |||
| "doc:lint": "remark .", | |||
| "start": "node packages/udaru-server/start.js", | |||
| "start-v16": "node packages/hapi-auth-udaru-16/start.js", | |||
| "start": "node packages/hapi-auth-udaru/start.js", | |||
There was a problem hiding this comment.
discussed on slack, but let's have one start, pointing to node packages/hapi-auth-udaru-16/start.js
| ``` | ||
|
|
||
| ## Usage | ||
|
|
||
| ```js | ||
| const Hapi = require('hapi') | ||
| const UdaruPlugin = require('@nearform/udaru-hapi-plugin') | ||
| const UdaruPlugin = require('@nearform/hapi-auth-udaru-16') |
There was a problem hiding this comment.
yes I much preferred the old naming convention, I know it's not hapi convention but I don't care. Am leaning back towards:
- udaru-core
- udaru-hapi-16-plugin
- udaru-hapi-plugin
- udaru-hapi-server
Thoughts?
| @@ -0,0 +1,65 @@ | |||
| 'use strict' | |||
|
|
|||
There was a problem hiding this comment.
we'll likely have to support both plugins for some time, anything that cuts down on duplication would be welcomed
I like this. |
|
I would recommend to ship https://www.npmjs.com/package/swagger-ui-dist instead of copying the files of swagger ui. It can be a separate issue, but I see the files are being moved around in this PR. |
|
I've update the package names as @dberesford asked. udaru-hapi-server now runs on udaru-hapi-16-plugin. |
|
Looks good to me Paolo! |
Fixes #501.
In order to maintain both v16 and v17 packages I created a new package called hapi-auth-udaru that contains both udaru-hapi-plugin and udaru-hapi-server as a single package.
The code has been slightly reorganized and simplified. Test coverage is 100%.
The udaru-core has been modified to support async/await, but its still compatible with hapi v16.