router: add async route handler support

[defunctzombie]

While the async/await feature is still highly experimental and not official, it does have preliminary support in tools like babel. There are even blog posts [1] about how to use the async/await feature with express routes.

However, these posts along with issue #2789 and #2788 show that using async/await is still a bit of a manual process and not clear. This change inspects the return value of a route handler to see if it is potentially a pomise. If the return value appears to be a promise then we attach to the error handler of the promise via .catch()

Now a user who is using babel can write the following without needing any wrap functions as most examples to date require.

app.get('/', async (req, res, next) => {
    let user = await User.findById(); // assuming .findById() returns a primise
    let org = await Org.findById();
    res.json({
        user: user,
        org: org,
    });
});

[1] https://strongloop.com/strongblog/async-error-handling-expressjs-es7-promises-generators/

---

Note on testing:
I have not added any tests since it would require instrumenting with babel but I can do that if the current maintainers think it would be useful. Alternatively, a fake test that just passes a promise like object to a route could simulate the same results.

Alternative implementation:
If the conditional checking is not desired in the hot path, then a wrap function could be applied upon adding the actual route versus at runtime. Additionally, that would open up the possibility of putting this behind an express setting which I didn't want to do in the hot path but maybe that would also be ok.

[defunctzombie]

/cc @dougwilson @jonathanong for feedback

[defunctzombie]

Also, this does provide benefits outside of async/await. You can just return a promise from any route handler and express will wire up .catch() for you automatically so you get the error handling you want without having .catch(next) everywhere. Maybe that is a win, maybe that is too much magic shrug.

[dougwilson]

The file you changed does not exist in Express 5, and I would assume this would be a 5.0 feature, correct? Would it be more advantageous to make the PR against the router npm module instead?

[defunctzombie]

@dougwilson actually I was targeting this for Express 4 (since it is completely backwards compatible and usable without also upgrading to v5); have not looked much into Express 5 or the router repo.

[defunctzombie]

If v4 is frozen then I am certainly up to do the change against the router repo if that is what will be used going forward. Just surfaced this based on my current usage (which is all v4).

[dougwilson]

We know it is backwards-compatible, but including promise support with the major is much more straightforward for users to understand if they can use promises and if they can use promise-based middlewares by simply knowing it as 4 vs 5.

[dougwilson]

One of the purposes of splitting the router to the router module is also because you can then use, say, router 2 with promises in whatever version of Express you prefer without coupling the upgrade together.

[defunctzombie]

@dougwilson gotcha, ok so I should make the PR against router or do you already have a branch that handles promise based middleware/want a different approach?

[dougwilson]

I know there have been a few people proposing ideas, but don't remember where they all are at the moment. @blakeembrey was telling me he wanted to work on this in a Skype message the other week as well.

[jonathanong]

trying to think of weird cases with double callbacks and when you'd ever want to await next(), but i can't think of any right now...

[blakeembrey]

I haven't continued working on it just yet, but the primary thing for promises to handle personally would be an automatic error handle (mostly here, but you need to handle falsy resolved promises in this PR) and upstream support (so next can itself return a promise).

[listepo]

+1

[xpepermint]

+1

[frogcjn]

Any progress here? Without promise support, using wrapper function is so ugly!

[StyleT]

@dougwilson Since this PR is backward compatible, why it can't be merged into v4? Without these few lines of code we have a LOT of pain & bugs with promises/generators in route handlers.

[garkin]

I'm not always monkeypatching.
But when i do it's Promise support for express.

[blakeembrey]

Promise-support is technically not backward compatible - for instance, it might break anyone like @garkin who has monkey patched it. If the error from .catch is being propagated forward, it could result in two next(err) calls. Anyone that's currently returning a promise and expecting it to not be handled could be the same.

I agree it's not ideal, but additional comments aren't really going to change the reality of it.

[blakeembrey]

Also, what I'd suggest doing since development is slowed and there's a bit of a bottleneck here, is building an Express-compatible router and using that until native support arrives. It's pretty easy to build a compatible router, it only needs to conform to (req, res, next) - everything else can be sugar, including async support.

[StyleT]

@blakeembrey Can you please point to some example/doc about usage of custom router in express?

[garkin]

I bet a single thing that people really want from express 5.0 is to take a modern approach for async handling without need to finally migrate from retrograde frameworks to koajs. But we won't see any progress for this for the next year at least.
This fruit is a low hanging one. All what you really need to do is:
https://gist.github.com/garkin/39e55d74c486cd75141d3d6721091cb2

This is called monkey patching not because monkeys are smart. As @blakeembrey correctly noted above, you need to know what are you doing.
When you return something from (...) -> void callbacks - you shoot your own leg.
If you have been caught in such antisocial behavior before - check your middlewares and routes.

/**
 * Code from:
 * https://github.com/expressjs/express/pull/2809
 *
 * Target Express version: { "express": "^4.14.0 <5.0.0" }
 * 
 * This monkey patch allows to return Promises from middleware and route handlers, rejections will be handled automaticaly.
 * 
 * Please note:
 *  If you return something from `(...) -> void` callbacks - you shoot your own leg.
 *  This could lead to double error handling if your callback handles error and returns Promise same time.
 *  So, please check your middlewares and routes for abnormal `Promise` returns before applying patch.
 * 
 */


var PACKAGE = require('express/package.json');
if (parseInt(PACKAGE.version.match(/^\d+/)[0]) !== 4) {
    throw new Error("This patch could be applied only for Express 4. " +
        "Express 5 will get it's own Promise handling.");
}

var Layer = require("express/lib/router/layer");
/*
    // https://github.com/expressjs/express/blob/9302acc5e449768b3ca2b03701d5379d86453af5/lib/router/layer.js
    // Target Function:

    Layer.prototype.handle_request = function handle(req, res, next) {
        var fn = this.handle;

        if (fn.length > 3) {
            // not a standard request handler
            return next();
        }

        try {
           fn(req, res, next);
        } catch (err) {
           next(err);
        }
    };
*/

if (!Layer.prototype.handle_request) {
    throw new Error("Something terribly wrong just happened, " +
        "there are no `Layer.prototype.handle_request` to apply patch to.");
}

Layer.prototype.handle_request = function handle(req, res, next) {
    var fn = this.handle;

    if (fn.length > 3) {
        // not a standard request handler
        return next();
    }

    try {
        var maybe_promise = fn(req, res, next);
        if (maybe_promise && maybe_promise.catch && typeof maybe_promise.catch === "function") {
            maybe_promise.catch(next);
        }
    } catch (err) {
        next(err);
    }
};

[olalonde]

@blakeembrey there's a few issues and PR regarding support async support. is there an official roadmap or status for this?

[bonesoul]

Can we get an update on this please?

[olalonde]

@defunctzombie is this ever going to get implemented?

[defunctzombie]

Not by me it isn't :)

[olalonde]

@dougwilson any updates on this?

[danielgindi]

This would be great, now with node 7&8 supporting async/await - for a much cleaner code.

[p3x-robot]

Node v8 will be LTS in October so async/await is a default function, I think we should add in asyn/await, if it will not be working with Express asyn/await, then Express is a dead end.

await/async is so awesome and it is soon in LTS, so everyone is using it, No more Promises, just try/catch/await/async and sometimes promises but we are not allowing to go forward with this dead end.
Who doesn't need await/async? They can use the old versions if they want the hell callback, and we could use v5 or v6 without the hassle of callback hell and complex functions.

This is like stopping evolution, and look at npm the fastest evolution in the world.
So Express will be a stop dead end, isn't evolution the most exciting thing?

[awerlang]

@p3x-robot have you seen the patch above from @garkin? It works great, use it and move on.

[p3x-robot]

Do you thing @dougwilson just simply is not interested in Express anymore?
@awerlang where is it this @garkin patch or to use it? I use the latest v5 existing but it no in this tag/branch.

[dougwilson]

I'm going to lock this thread. The PR is failing, and has had follow up PRs on the router repo ehich express 5 uses. Please read the entire thread.

[dougwilson]

Really, since Express 5 is based on the router module and some very nice PRs have been made there, it is best to track this in a single location to prevent cobfusion from an idling pull request.

Discussions and pull requests for Express 5 (and thus promise support OOB) should ideally be done in https://github.com/pillarjs/router