Return middleware chain promise from callback()

[PlasmaPower]

Without this, there's no way of hooking into after Koa is done with the response. This can be extremely important if used with ctx.respond = false, as in #846 (see there for use case). While this is technically a breaking change, I don't expect it to break anything.

[dougwilson]

i.m.o. it's not really a breaking change :)

[PlasmaPower]

Yeah, the one situation I could see this breaking anything is if (handler(req, res)), but that shouldn't happen (hopefully). http://xkcd.com/1172/

[codecov-io]

Current coverage is 100% (diff: 100%)

Merging #847 into master will not change coverage

@@           master   #847   diff @@
====================================
  Files           4      4          
  Lines         417    419     +2   
  Methods        81     81          
  Messages        0      0          
  Branches      102    104     +2   
====================================
+ Hits          417    419     +2   
  Misses          0      0          
  Partials        0      0          

Powered by Codecov. Last update c62aa90...734a62c

[PlasmaPower]

Current coverage is 100% (diff: 100%)
Merging #847 into master will not change coverage

I don't think that's how diffs work... 😄

[jonathanong]

can we add a test for the use-case if we add this? not opposed to it.

[bttmly]

This is great! One thing to be aware of is that even if you do ctx.respond = false and then use the promise-returning API from this PR to do further processing after koa finishes with a request, there is still the fact that koa has set res.statusCode = 404. It's not difficult to reset that value, but you risk overwriting something in the koa app explicitly setting status code to 404. Is there a way for koa to internally mark a request as "maybe send 404 if respond is reached and ctx.respond is not false" without actually changing the state of the res object? Making koa v2 "play nice" with existing connect/express apps will, I think, provide a much more compelling migration story, especially for large production applications which, by necessity, need to migrate incrementally.

[jonathanong]

Is there a way

yes :) suggestions welcomed. i haven't thought about this yet, so feel free to make suggestions.

[PlasmaPower]

Maybe app.usedAsMiddleware? It would remove the default statusCode setting and return early here (don't res.end() if there's nothing to send).

[PlasmaPower]

Okay, I've just added in app.usedAsMiddleware. Let me know how it looks and I can port it over to the v2 PR.

[jonathanong]

im ok with it. anyone else? @koajs/owners

though maybe we could use a name with only 1-2 words :P

[PlasmaPower]

Yeah a better name for the app variable would be appreciated.

[PlasmaPower]

I can rebase and pull the changes into the v2 branch if you're ready to merge.

[bttmly]

Any updates on this?

[PlasmaPower]

If a better config name is needed, maybe disableDefaultResponse?

[EduardoRFS]

why not àpp.middlewareMode or app.mode = 'middleware'

[PlasmaPower]

Because that describes a common use case, not its function.

[bttmly]

So as I look at this again, it occurs to me that we may only need to return the promise from app.callback() and everything else can be implemented in users' code:

const app = new Koa();
// very first thing we do, set `ctx.status` to null 
// very last thing we do is check if `ctx.status` is still null
app.use(async (ctx, next) => {
  ctx.status = null;
  await next();
  if (ctx.status == null) ctx.respond = false;
});

// main app logic and routes here -- in this simplified example, 
// Koa is just supposed to handle a single route `/user`
app.use(async ctx => {
  if (ctx.url !== "/user") {
    return; // let ctx.status remain `null` -- defer handling to enclosing app
  }
  // assume `ctx.session` has some relevant data, elided here
  const user = await db.getUser(ctx.session);
  ctx.body = user;
  ctx.status = 200; // <-- set status here, means Koa is responsible for sending response
});

module.exports = app;

usage from existing express/connect app

const expressApp = express();
const koaCallback = require("./myKoaApp").callback();
expressApp.use(function (req, res, next) {
  koaCallback(req, res).then(function () {
    if (res.headersSent) return;
    next();
  });
})

Pros:

• Avoids adding any configuration fields or extra documentation.

Cons:

• Offloads onto user the need to implement the middleware shown above.

Alternately -- which I think is less-good than above but maybe better than usedAsMiddleware we can have a field app.defaultStatus with an initial value of 404, and use that to set initial ctx.status, so user can override that value (like with null -- basically equivalent to what I did here, but the user's code still needs to check ctx.status == null at the end of the middleware stack).

Thoughts?

[bttmly]

@PlasmaPower @jonathanong any thoughts on the comment above? would love to get support for embedding Koa into existing applications merged ASAP. Happy with the current implementation if people feel it's superior to what I proposed above.

[jonathanong]

@nickb1080 yes, that looks feasible. It's been a while since I looked at this issue. if all we want to merge is return fn.call(ctx).then(...), then I'm okay with that

[bttmly]

@jonathanong How do you want to proceed? Do you want me to open a PR against v2.x with the single small change of adding return?

[fl0w]

@nickb1080 why not master?

[PlasmaPower]

Why does the branch v2.x still exist? It seems to be just a previous point on master.

[bttmly]

Ah! I hadn't noticed the switchover had already happened. This PR (847) has some extraneous stuff that doesn't seem necessary -- the simple change in https://github.com/koajs/koa/pull/848/files would be sufficient I think.

[PlasmaPower]

Yeah I agree. Closing this in favor of #848.