es7 async function feedback #415
Comments
|
👍 |
|
🍻 |
|
This looks awesome. I did some dabbling with a different composing mechanism with only promises and async await. Thought maybe I should post it here for feedback function compose (middleware) {
let responses;
let next;
middleware = middleware.map((mw, i) => {
return function (){
next = () => (middleware[i+1] || noop).call(this)
return responses[i] = mw.call(this, next)
};
});
return function composed () {
responses = [];
return middleware[0].apply(this, arguments)
.then(() => Promise.all(responses));
};
}Using babel with Thoughts/Criticisms? |
|
I wouldn't want to change the Perf-wise I'd have to see some numbers! That's not really a huge concern though, less deps/complexity is a nicer win. We're already achieving numbers that would sustain most applications with a single node, and no production app will have a single node so that's less of a concern. |
|
@tj Hah, good point...
This could be quite different in versions of io.js (for native promises) I'm thinking I can still short-circuit with this mechanism too.. I should mention that this pattern is not my own. Its copied from the way OWIN and Katana (.NET) handle their async await middleware. |
|
Next probably won't be going anywhere, having proper middleware is a lot more flexible than the Express style, but use whatever works for you obviously! |
|
@jonathanong Just wanting to get started doing some performance testing and having issues getting the new syntax to work: var koa = require('koa');
var app = koa();
app.experimental = true
// logger
app.use(async function (next){
var start = new Date;
await next;
var ms = new Date - start;
console.log('%s %s - %s ms', this.method, this.url, ms);
});
// response
app.use(async function (){
this.body = 'Hello World';
});
app.listen(3000);Is this supposed to work? Or did I misunderstand something with the syntax? |
|
What error are you getting? |
|
Running on io.js 1.2: /Users/markus/Projects/Artificial/koa-test/index.js:19
app.use(async function (next){
^^^^^^^^
SyntaxError: Unexpected token function
at exports.runInThisContext (vm.js:53:16)
at Module._compile (module.js:429:25)
at Object.Module._extensions..js (module.js:464:10)
at Module.load (module.js:341:32)
at Function.Module._load (module.js:296:12)
at Function.Module.runMain (module.js:487:10)
at startup (node.js:111:16)
at node.js:799:3 |
|
sorry, instructions are probably unclear. you need to use babel to enable https://github.com/koajs/koa/blob/master/test/experimental/index.js |
|
Ahhhh, I was almost thinking that. But also a bit surprised because |
Yes |
|
Ok thanks all working now - let the happy testing begin :) |
|
@calebboyd would you happen to have your benchmark code laying around? I'm curious as to why asyncToGenerator is so much slower than plain koa since the two should be doing roughly the same thing. As I understand it, both wrap native generator functions to create functions that return promises, using the built-in Promise class. |
|
@ide the benchmark I had above used promise based middleware instead of the included genereator based. The comparable performance I believe is due to koa using plain generators (not wrapped, good performance). Where asyncToGenerator wraps generators in promises, which is then wrapped again by the composition function. But when using promise based mw and bluebird, intermediate promise and closure allocations are avoided (bluebird optimization). |
|
Thanks, guys. |
|
@calebboyd any chance that example can go public on github? |
|
@mgenev What are you looking for exactly? The basic gist is posted above. The problem is that it breaks koa's generator based api and consequently anything implemented with it :/ . mho as expressed in #420 Is that promise based middleware should be used if koa uses async await. As it seems we might eventually get I can probably add a fork with the changes... |
|
@calebboyd I got a lot of it working with bluebird coroutines and the composition from the OP, but when i start routing I'm getting confusing errors like I also tried with koa-route, but it also breaks with async functions, I'm guessing both of those modules have to be rewritten for async/await? |
|
Better use something like Earl Grey: http://breuleux.github.io/earl-grey/ while the native option is unavailable |
|
You can use babel on the server side to get async/await. (i have not done this though, so I can't be of much help). |
|
Was playing around with async/await in koa, I keep getting None of the routers work well with async functions :(. I'm using koa-router, and as a stopgap I'm doing: const routes = new Router().get('/foo', compose([a, b, c])) |
|
@cesarandreu That was my experience as well, so I opened an issue which is referenced above, but so far no response. It's kind of a silly situation because I've ben able to use async/await in an express app, but not koa which is built for the future JS |
|
@cesarandreu Could you share more on what you are actually doing in |
|
nice :D |
|
This looks really good! Nice work. app.use(async next => {
await next()
}); |
|
@ruimarinho i don't believe that'll work because |
|
Hmm, it did, because I've just tested it with |
|
Ah boo I was thinking it was just lexically bound by default, couldn't remember, guess not. Maybe passing ctx is the way to go then. Need a skinny arrow :( moar ES features |
|
One thing I like about passing class KoaResponseCompressor {
constructor(options) { ... }
async compressResponseAsync(ctx, next) {
await next(ctx);
...
this.helperMethod();
...
}
helperMethod() { ... }
}
export default function middleware(options) {
return ::new KoaResponseCompressor(options).compressResponseAsync;
}import responseCompressor from 'koa-response-compressor';
app.use(responseCompressor(options)); |
|
@ide good point. It's definitely the more elegant solution, arg or not. Only other alternative I can think of at the moment (which I don't like haha) is hanging it off |
|
Really neat @ide Just a note on some specs... Because of that, you can't use native @tj, The composing function I set up implements both of those options |
|
+1 for passing ctx as an argument:
For me koa with a more functional style (this change and maybe without prototypes) and async functions will be close to perfection :P |
|
Just realized that using ctx as argument would work very nice with destructuring. app.use(async ({request, response}, next) => {
// ...
await next()
// ...
})Not sure if this is really useful, but worth sharing. |
|
koajs/compose#27 if anyone wants to check it out |
|
What's the strategy to create compatibility with existing middleware that doesn't expect a context as an argument? |
|
For the compatibility, And when using non-GeneratorFunction, next is instance of Promise is easy to understand, i feel. app.use(function *(next) {
this.body += ':b';
yield next;
})
app.use(async (next, ctx) => {
ctx.body += ':D';
await next;
});
app.use(async function(next) => {
this.body += ':)';
await next;
});
app.use((next, ctx) => {
ctx.body += ':D';
return next;
});
app.use(function(next) => {
this.body += ':)';
return next;
}); |
|
@hiroqn , the argument order is set. Context is passed first because it is required and next is optional. Not binding to this allows comments like @ide's above. As for making next a promise, next is a middleware function. It has to be invoked. Your examples never invoke next. Indeed, they seem to rely on the next middleware having already been invoked. This doesn't makes sense. return next(ctx); This will still return a promise. |
|
I really think the concept of calling the next middleware function explicitly and awaiting it's promise is much easier to understand for newcomers than yielding next without calling. |
|
@jonathanong, can koajs/compose#27 wrap up into some pre-release version right away and then provide a koa pre-release branch that uses the koa-compose pre-release? Having this available ASAP would help it mature into something releasable. |
|
@smcmurray @jonathanong what about releasing it as 3.0.0-alpha on npm? |
|
I would be very happy about a 3.0.0-alpha release of this right now: I'm setting up the foundation for a project that won't see production for a little bit but needs to build developer consensus sooner rather than later. I'm a pretty big fan of this proposal, so I'd love to start out in this direction! |
|
Once it ships I'm using it! |
|
please test this! #530 |
|
Is still there a ctx.state? |
|
not related to es7 async functions. if there isn't, you could always do |
and others
es7 async functions are now supported when you enabled
app.experimental = true. it uses https://github.com/thenables/composition. however, the last time i benchmarked it, it was significantly slower.please provide feedback and maybe do some benchmarks and performance improvements so we can one day support es7 async functions by default without sacrificing anything :)
The text was updated successfully, but these errors were encountered: