-
Notifications
You must be signed in to change notification settings - Fork 16
feat: async await and upgrades Hapi to v18 #35
Conversation
BREAKING CHANGE: All functions that took callbacks now return promises
dfe834c
to
946e8a1
Compare
Includes the changes from #7 |
package.json
Outdated
"hapi": "^16.6.2", | ||
"inert": "^4.2.1", | ||
"libp2p-crypto": "~0.16.1", | ||
"epimetheus": "achingbrain/node-epimetheus#hapi-18-support", |
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.
Pending merging of roylines/node-epimetheus#63
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.
Thanks for the PR @achingbrain
All in all, it looks good. Left a minor question
@@ -101,17 +101,14 @@ function Protocol (log) { | |||
} | |||
|
|||
function getIdAndValidate (pub, id, cb) { |
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 not doing async await here as well?
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.
It didn't appear to be part of the public API and is only invoked in the context of a socket.io-pull-stream
handler which is all callbacks.
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.
Looks good, waiting for merge of upstream module
If this doesn't get merged in a timely manner, should we just use a fork instead?
Ok, merging then |
I'm not sure - is it required for the package? Do you have it enabled? Aegir will prompt you for an OTP automatically if it detects the challenge in the response. |
Unsure and no |
I think you can check if you log in to the npm website: https://docs.npmjs.com/requiring-2fa-for-package-publishing-and-settings-modification Also, you should enable 2FA 😉 |
I've enabled 2fa for my account and set it to authorization only. Same error, no clue why. Created a tarball using npm (in case any generated files are needed that aren't in the repo) @daviddias Could you try publishing this? (zip includes a tarball created by npm and a signature for that tarball) Edit: I've contacted support about this, but it's weekend, so I'll have to wait until monday. |
FYI I am looking into some issues with this. There are internal usages of crypto that were not migrated from callbacks to async/await with the latest version bumps causing crypto handshakes to stall and fail. |
BREAKING CHANGE: All api functions that took callbacks now return promises
Still uses pull-streams internally because if it ain't broke you shouldn't try to fix it.