Support catching rejected promises in middleware functions

[ORESoftware]

I discovered this:

https://medium.com/@the1mills/hacking-express-to-support-returned-promises-in-middleware-9487251ca124

seems to work so far. I looked at the latest releases of Express 4 and 5, and doesn't seem to be supported.

Would it be possible to handle returned promises in middleware functions - at least if the promise is rejected?

[dougwilson]

This will be supported in Express 5, yes. It is unfortunate, but many people alreay are returning promises from their route and middleware functions, and not expecting Express to act on that (because it never has before) which is why it is a semver major change to just have it there.

Maybe an option is to make it so you can indicate in some way that your function that returns a promise wants Express to handle the rejections, though.

Are you OK with the plan yo land in 5.0? Or do you have some thought on how we can safely land it in 4.x without causing it to listen to rejections for existing code out there?

[ORESoftware]

@dougwilson the only way that I can think of to have it land in 4, is to have a PromiseRouter instead of instead of just Router, and the PromiseRouter has the alternate Layer methods that handle returned promises.

The other challenge is - does express just catch errors, or could express also automatically call next() even if there is no error? I recommend just the former.

[dougwilson]

is to have a PromiseRouter instead of instead of just Router, and the PromiseRouter has the alternate Layer methods that handle returned promises.

Do you mean something like https://www.npmjs.com/package/express-promise-router ?

[ORESoftware]

@dougwilson express-promise-router is just a crappy inefficient wrapper around express (tmk). What I mean something like this:

express.Router({promise:true});

and then in Layer.js, all you just have to do this check:

  try {
    var p = fn(req, res, next);        // exhibit 2B
    if (this.isSupportPromise && isPromise(p)) {
      p.catch(next);
    }
  } catch (err) {
    next(err);
  }

that's more performant and also less code changes AFAICT. Like I said, automatically calling next() on behalf of the user is not that useful, but that's something else to consider. My code obviously only calls next() if there is an error, which I think is the better way to go, but I dunno.

[dougwilson]

Yea, that is what is designed for Express 5 👍 it turns out many people use other middleware in their routes that may or may not return values like promises on accident.

Your other question is another issue that hasn't turned up a good discussion. The catch + next is landing in Express 5 and if you would like to join in the conversation for the other aspects, feel frer to join in the already open issues/ PRs 👍

[dougwilson]

Duplicate of #2259 . If you don't think it's a duplicate, please describe why and what the difference is and we can update the topic and reopen for this different discussion 👍

[ORESoftware]

Sounds good.

Yeah you shouldn't automatically call next() for the user, here is why:

router.use('/foo', function(req,res,next){
  return Promise.resolve().then(function(){
       doSomeCallbackThing(function(err, v){
           err ? res.json({error:err}) : res.json({success: v || null});
       });
   });
});

if next() is called automatically for the user when the returned promise resolves then doSomeCallbackThing won't have time to complete, etc etc.

Also, I don' think you need to await promises, just need to use .catch().

[dougwilson]

Right, and even then your example above of p.catch(next); has the issues where return Promise.reject() (a reject without a reason) will end up being the same as a blank next() and thus just continues without an error ever getting raised. We want to add this and there are a lot of edge cases like this and you'd be surprised how similar a PR here which covers all the cases ends up looking very similar to https://www.npmjs.com/package/express-promise-router as it has been done a few times already.

[ORESoftware]

ok cool, I trust that yall will do the right thing! :) I think automatically calling next for the user is the one very bad idea lol. I think there is a big benefit for people to avoid including async/await in their codebases. Avoiding async/await for middleware functions was one of my goals when finding my solution.

[dougwilson]

I think automatically calling next for the user is the one very bad idea lol.

Yea, after various proposals, I think that is the conclusion thus far. Maybe some future version of Express you can actually resolve a response, but until you can just handle the whole flow with reject / resolve having a half thing is just too surprising we feel at this point.