-
Notifications
You must be signed in to change notification settings - Fork 38
Basic Support for Async/Promise-Based Middleware #96
Conversation
This goes along with krakenjs/kraken-js#495 and is a proof-of-concept at this point, not really production ready. There are a few potential ways to support promise-based middleware. This is only one of them.
Okay I've tested this and it works great locally. Not exactly sure how to test for memory usage and other things, but I'd like some serious feedback. Also as to which versions of node.js we need to support. |
return registry; | ||
|
||
}; | ||
|
||
function makePromiseFriendly(registry) { | ||
let methods = ['use', 'get', 'post', 'delete']; |
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.
what methods am i missing?
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.
options
, head
, put
, trace
, connect
if you want to include all the possibilities from HTTP/1.1
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.
👍
let oldMethod = registry[method].bind(registry); | ||
registry[method] = (...routes) => { | ||
oldMethod(...routes.map(x => { | ||
if (typeof x === 'string') return x; |
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.
are the only possible things here strings, arrays and functions?
}) | ||
} | ||
|
||
function makePromiseAware(x) { |
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.
probably deserved a comment or somehting
can we avoid spread to allow support for node@4? |
@xjamundx thoughts on having another package called |
Maybe we could couple this with a major version bump of kraken and say it's v6+ or even v8+, depending on what else we may need/want to accompany v8 support? |
Love the idea of doing a node 8 refresh of kraken (kraken....4?) with built in support for aysnc middlewares, full removal of bluebird, etc |
In the meantime yeah @gabrielcsapo I'd totally consider a separate package, should be reasonably easy to swap out. |
This goes along with krakenjs/kraken-js#495 and is a proof-of-concept at this point, not really production ready. There are a few potential ways to support promise-based middleware. This is only one of them.
Current PR only works in node >= 6.