Skip to content
This repository has been archived by the owner on Jan 25, 2020. It is now read-only.

Basic Support for Async/Promise-Based Middleware #96

Closed
wants to merge 1 commit into from

Conversation

xjamundx
Copy link

@xjamundx xjamundx commented Sep 22, 2017

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.

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.
@xjamundx
Copy link
Author

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'];
Copy link
Author

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?

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

Copy link
Author

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;
Copy link
Author

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) {
Copy link
Author

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

@gabrielcsapo
Copy link
Contributor

can we avoid spread to allow support for node@4?

@gabrielcsapo
Copy link
Contributor

gabrielcsapo commented Sep 26, 2017

@xjamundx thoughts on having another package called express-enrouten-async?

@grawk
Copy link
Member

grawk commented Sep 26, 2017

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?

@xjamundx
Copy link
Author

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

@xjamundx
Copy link
Author

In the meantime yeah @gabrielcsapo I'd totally consider a separate package, should be reasonably easy to swap out.

@gabrielcsapo
Copy link
Contributor

gabrielcsapo commented Sep 27, 2017

@xjamundx @grawk node@8 refresh of kraken would be awesome! full removal of bluebird 🙏 please! If we make this a separate package merging that into kraken would be easy (for the refresh), everything could be in core also!

@xjamundx xjamundx closed this Mar 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants