async (ctx, next) => await next()

[jonathanong]

goal is to be able to do:

app.use(async ({request, response}, next) => {
  response.body = await somethingAsynchronous()
  await next()
})

TODO:

• guard against multiple next() calls
• standardize because the styling is over the place
• squash and merge

[mpal9000]

What do you think about moving node's req, res one level deeper eg. ctx.node.req and renaming koa's request, response to req, res (top level)?

[jonathanong]

that's a big API change. you could always just use ctx...

[mpal9000]

OK thanks anyway, just thought it was the right time to ask, because you are considering some more API changes.

[gyson]

it returns function (context) { ... }, but this is not middleware.

[jonathanong]

@gyson oh true. it should be function (context, next) {}

[gyson]

@jonathanong Nice!

[drewhamlett]

awesome

[AutoSponge]

I added a PR which should resolve the coverage issue. Additionally, I realized it's not possible to hit the catch leading to Promise.reject. The promise will reject when the error is caught and invoke the thenable.catch handler.

[gyson]

try...catch... is necessary for non-async case, like this:

app.use((ctx, next) => {
  throw new Error()
})

[jonathanong]

@gyson i agree, but when would that actually happen?

[smcmurray]

@gyson, is (ctx,next)=> {throw new Error()} valid middleware?

[gyson]

@jonathanong fn(context, function next() { ... } will throw an exception, which results next() in previous middleware throw this exception instead of return a rejected Promise.

[AutoSponge]

@gyson I think I see the issue. I'll add it back with a test case that covers it.

[gyson]

@smcmurray Yes. It's valid.

A middleware is a function with two arguments.

First context is a Koa context instance.
Second next is a function which will invoke next middleware. Call next() to invoke next middleware and it should always return a promise instance (either resolved or rejected).

The middleware function could return any value (include Promise instance) or throw exception.

1. If it returns a value, the value will be wrapped as a resolved promise by Promise.resolve.
2. If it throws an exception, the exception will be wrapped as a rejected Promise.

This wrapped promise will be the returned promise of next() in previous middleware. This wrapping step is to ensure that next() will always return promise.

async (context, next) => { ... } is just a special case that it will always return promise.

Hope this helpful. Let me know if it's still confusing : )

[AutoSponge]

I added a PR to correct my mistake. It has the try... catch... back and it has 100% coverage.

[calebboyd]

Looks neat, You could probably avoid the extra closure (dispatch) per call by wrapping them all up front...

[smcmurray]

Does the merge of PR #29 close this? Or is there more to do?

[jonathanong]

I want to keep this open for a little bit for feedback

[fundon]

ES6+, supports this?

app.use((({req, res, session}, next) => {})

Updated: ({req, res, session}, next) => {} is fine.

[calebboyd]

@jonathanong What are your thoughts on including next as a context property as well?

[jonathanong]

@calebboyd sure!

[jonathanong]

PR welcomed until I get to it!

[calebboyd]

Great, #30, I threw in some other stuff too, relating to my earlier comment

[gyson]

Just personal opinion, context and next should be independent with each other.

context is kind of a global state shared by all middleware and next is just a local function bind to one middleware. Mix global context and local next may be confusing.

For example,

app.use(async ctx => {
  await ctx.next()
  await ctx.next() // no idea what will happen because `.next` is changed now.
})

[calebboyd]

@gyson the opposite could also be said imho. That next is just like an instance method that applies the next middleware's functionality to the request state (context). Almost like the middleware are effectively mixins for the request state.. Its also consistent with the iterator or generator pattern where invoking next will have different effects after every invocation. (until it has none --- done)

The above code could be considered a bug just as forgetting to use await could be a bug

I think there is a TODO in for invoking next multiple times, though different from the original implementation would still be valid when setting next on the context

[gyson]

About next as context property

Pros:

you can do:

app.use(async ({ request, response, next }) => {
  await next()
})
// or 
app.use(async ctx => {
  await ctx.next()
})

Cons:

1. occupy .next property of context
2. call ctx.next() multiple times result unpredictable outcome. also I don't see a feasible solution to guard multiple ctx.next() call
3. you have two more ways (from Pros) to invoke next middleware. I think only has one way to invoke next middleware is better because it's simpler and less confusing.

Thanks for your effort. But in this case, I think Cons > Pros.

About #30

the implementation has a bug (not sure why tests did not check it out).

It does not work with following case:

compose([
    compose([
        (ctx, next) => {
            console.log(1)
            return next()
        },
        (ctx, next) => {
            console.log(2)
            return next()
        }
    ]),
    (ctx, next) => {
        // it never reach here !
        console.log(3)
        return next()
    }
])({})

[calebboyd]

Ahh nice, I added a test. Con 1 is the goal, so you've already stated you don't like it :P. Con 2 is still valid either way. Nothing will stop the user from writing incorrect code. (the side effects are different, but still there none the less). Con 3.. its really only one more way, just depends on whether or not the user chooses to use destructuring for the arguments. If this is largely unliked it doesn't have to be there... I thought I'd just toss it out, Conceptually, it makes a lot of sense to me being that the context acts like an iterator. But I'm not trying to paint the shed red by any means :)

[calebboyd]

@jonathanong I think he was talking about #30 which I just added the same test to

[gyson]

I don't think you can have both ctx.next() and guard calling multiple times.

compose([
  async ctx => {
    await ctx.next()
    await ctx.next() // it won't throw
  },
  async ctx => {
    // do nothing
  }
])

[jonathanong]

Conceptually, it makes a lot of sense to me being that the context acts like an iterator.

@calebboyd hmmm this is a good point... not sure about calling the function ctx.next... you should be doing ctx[Symbol.iterator] to get the iterator anyways but, still, this may confuse people. could just call it done or down or something

[jonathanong]

@gyson added another test. i don't see it failing.

@calebboyd was trying to cherry pick your tests but couldn't give you credit :( then i forgot i had it locally and committed it lol sorry

[calebboyd]

Thats all good it was @gyson's code

[gyson]

no. please test await ctx.next(), not await next()

[jonathanong]

@gyson great! thanks!

[jonathanong]

not sure i'm liking ctx.next()! lol

[calebboyd]

I guess that's what we get when it's only partially an iterator :p (no current). The whole thing is doable with mw so it can be scrapped

[AutoSponge]

I have some feedback based on developing a microservice last week using shan and a bunch of koa/co-related plugins.

The inclusion of (ctx, next) in the function signature makes it very clear whether you're dealing with a middleware function or a response-ending function. I really like it.

// example response end using ioredis
export const read = async ctx => {
  const result = await ctx.redis.get(/* something */);
  if (result) {
    ctx.status = 200;
    ctx.body = {status, result};
  }
  //responds with default 404 unless something threw
};

// example middleware for body validation
export const validate = async (ctx, next) => {
  if (['PUT', 'POST'].includes(ctx.method)) {
    // ...schema validation
    ctx.assert(valid, 422, message);
  }
  await next(ctx);
};

[smcmurray]

It looks like the discussion here has wound down. As near as I can tell, the only questions left up in the air were whether context should be context or an argument, and whether next should be a property of context.

context vs argument

As I read it, there looks to be much more support for it being an argument. As far as I can tell, there's not any disagreement left on that point.

next as a property of context

Pros

• it makes context smell like a generator.

Cons

• It makes context smell like a generator when it really isn't
• It adds bloat to context
• It's redundant

I'd personally vote no on this issue, but it doesn't really matter to me.

Other

Are there any other issues that should hold this up from moving forward? How long does it need to be open for discussion? Can we move ahead ASAP and transition koa sooner than later?

[jonathanong]

@smcmurray sums it up :)

If you guys can bring awareness to this issue and see if anyone else has any input, that would be great!

I'm also not even sure if async functions have an arrow function form. We should double check that. I know Babel supports it, but I don't know if it's in the actual specs.

[AutoSponge]

I think this is the lastest. AsyncAwait and AsyncArrows

[gyson]

Only concern I have is about backward compatible ( implicitly convert generator middleware to promise middleware ).

//
// we can have this conversion in koa-compose, like following:
//
function compose(middleware) {
  middleware = middleware.map(mw => {
    if (isGeneratorFunction(mw)) {
      return convertGeneratorToPromise(mw)
    } else {
      return mw
    }
  })
  // ...
}
//
// or we can have conversion within koa repo, like this:
//
app.use = function (mw) {
  if (isGeneratorFunction(mw)) {
    mw = convertGeneratorToPromise(mw)
  }
  // ...
}

function convertGeneratorToPromise(genFun) {
  return function (ctx, next) {
    return co.call(ctx, genFun.call(ctx, createGenerator(next)));
  }
}

function* createGenerator(next) {
  return yield next() 
}

[smcmurray]

I prefer the conversion in koa-compose.

[smcmurray]

I never did see any performance comparison with #30. I see @calebboyd closed #30, though. @calebboyd also backed out next as a a context property. So is support for legacy (generator) middleware the only issue left?

[calebboyd]

I think so!

On Tue, Oct 6, 2015, 10:24 AM smcmurray notifications@github.com wrote:

I never did see any performance comparison with #30
#30. I see @calebboyd
https://github.com/calebboyd closed #30
#30, though. @calebboyd
https://github.com/calebboyd also backed out next as a a context
property. So is support for legacy (generator) middleware the only issue
left?

—
Reply to this email directly or view it on GitHub
#27 (comment).

[smcmurray]

Not all middleware is chained through the 'use' method. Routers, for instance may not invoke use at all, but might invoke compose to build their own sub-chains. On the other hand, anything that does invoke 'use' also invokes compose indirectly. So converting generator middleware in compose makes more sense to me.

[gyson]

Created a stand-alone lib to convert generator-based middleware to promise-based middleware.

You can check it at koa-convert.

[jonathanong]

i would rather convert to generator middleware in koa or in the router vs in compose. ideally, this would be the last version ever, and eventually koa and routers can remove generator support

[smcmurray]

@jonathanong, wouldn't that mean that you don't have backwards-compatability out of the box?