Add support for throwing in async handlers.

[maxcbc]

Add support for throwing in async handlers without UnhandledPromiseRejectionWarning

What

• if a handler or error handler has a constructor name of AsyncFunction, it will pass next to the handler's .catch.

Why

This gives developers the freedom to throw again but also means they don't have to do the following pattern.

async function (req, res, next) {
  let result;
  try {
    result = await something();
  } catch (err) {
    next(err);
  }
  res.send(result);
}

They can instead do:

async function (req, res, next) {
  res.send(await something());
}

[blakeembrey]

Async functions are just wrappers around functions returning promises. There’s an existing PR support promises. Have you looked at that?

[wesleytodd]

#47

[maxcbc]

I may be wrong I think the solution in #47 would still log UnhandledPromiseRejectionWarning as the promise is not called with a .catch() but the .catch() is added later.

If this is the case I think (only my opinion) it should be avoided as it will fill up a lot of consumer's logs unnecessarily.

It may not be as usable across versions as promises are, but the nice thing about AsyncFunction is that you can detect that it returns a Promise before you call it, meaning you can call it differently if you need to.

[blakeembrey]

You should give it a go and report back. It would work because it’s within the same tick. Technically there’s no way to “call a promise with a .catch()”, it’s something you would attach after calling a function that returns a promise and node.js works on an event loop, so this is within the same event loop.

[blakeembrey]

Also, the same comments apply here - you can throw a falsy value and return a falsy promise. You also need to be aware how confusing this will be for consumers - by giving examples of async functions, people will definitely be using transpilers such as Babel or TypeScript. They will not tell you it’s an async function however, since they only transpile down to returning a promise, so this feature can produce different results depending on development practices.

[wesleytodd]

by giving examples of async functions, people will definitely be using transpilers such as Babel or TypeScript

This is not always true. I am actually working on my first production level api with node 10 and async/await right now. I am using the v4 router, so no async middleware, but I am running non-transpiled async functions. So the only real answer is to check typeof returnValue.then === 'function'.

Also, I will for sure start a beta version of my projects with router@2 and try out all of these things asap, maybe this weekend.

[blakeembrey]

Sorry, I didn’t mean to imply people will only be using transpilers, just that people many people transpile their code today using these tools, but depending on the emit output it will either work or not work for this PR.

[maxcbc]

@blakeembrey in the case of transpiled code you will always get UnhandledPromiseRejectionWarning either way because the constructor name will be different.
By doing it on constructor name it won't break transpiled code, but will add support in non-transpiled code.
But we may be able to do both so as to do a .catch as per #47.

i.e.

  if (fn.constructor.name === 'AsyncFunction') {
    return fn(req, res, next).catch(next)
  }
  try {
    var result = fn(req, res, next)
    if (result && typeof result.then === 'function') {
      result.then(null, function (error) {
      next(error || new Error('Promise rejected with: ' + error))
    })
  } catch(error) {
    next(error)
  }

[blakeembrey]

I know, that’s why I left the comment. You’ll notice the first response and PR is from myself. And you’ve validated the problem with using the constructor name, something that works on my machine might not work on your machine. For example, if I write a module using the router and publish it to NPM and I happen to be using the latest node, I’m now likely to get a bunch of issues with people not running the latest node who I then just have to point to here and say it’s a feature, not a bug. It’s not difficult to support promises first class instead, which was my comment.

I’m also still not sure something is missing in my explanation. You do not need the first line to check the constructor. It’s returning a promise anyway. The only reason I imagine doing that is by routing these functions down a different code path for performance reasons (async functions won’t throw synchronous errors), but that shouldn’t be done where you wrote it - it should be done once otherwise you have a small impact by checking this same code path every time.

[maxcbc]

@blakeembrey Apologies, I'm not sure I'm addressing all of your points in the below. I'll read over and have a think.

What I'm getting at is that I agree your PR supports promises across the board, which I think having looked at it needs to be done regardless of this PR.

What this PR has the added benefit of is that it will not raise any UnhandledPromiseRejectionWarning in consumers logs.
We're in a nasty situation where the below raises one of these warnings:

const prom = doSomethingAsync()
prom.catch(err => console.log(err))

but the following does not:

const prom = doSomethingAsync().catch(err => console.log(err))

All my PR, having reflected on yours adds is protection against these warnings. It is safe on old versions of node (tried on 0.10.x) because fn.constructor.name === 'AsyncFunction' will evaluate to false on versions of node that don't support async/await.

[blakeembrey]

@maxcbc Did you go and test it as I suggested in #69 (comment)? I have not replicated what you've said, and I don't believe I ever would be able to, because, as far as I understand, it works as I described in that same comment. I've attached this code and output as the example I've just run to demonstrate, please let me know if you see some fault in it.

async function test () {
    throw new Error('boom')
}

const result = test()

console.log(test.constructor)

if (result && typeof result.then === 'function') {
    result.then(null, err => console.log(err)).then(() => console.log('done'))
}

[blakeembrey]

In fact, you can't even wrap it in process.nextTick to replicate because of the way it gets enqueued. To replicate what you are saying you actually need setImmediate or setTimeout. More info available at https://nodejs.org/en/docs/guides/event-loop-timers-and-nexttick/.

[dougwilson]

I changed the branch base for this PR to 2.0, which is where the Promises / async support lives. @maxcbc following the conversation, you're saying that the 2.0 branch currently has this UnhandledPromiseRejectionWarning bug. I ran your tests in this PR against that branch without your changes and they passed. Can you add a test to this PR that asserts the bug?

[dougwilson]

Closing due to no follow up.

[adamreisnz]

Has any work on this been done in another place? A lot of closed issues refer to this PR, but now I see the PR has been closed due to no follow up.

Is Express 5 going to support async route middleware with throwing errors, or do we still need to wrap each middleware in a wrapper?

[wesleytodd]

#47

That is probably what you are looking for, and it is merged to the 2.0 router. Which in turn is released in the current beta for express 5.

There is also this one I opened just the other day: #75

[adamreisnz]

Brilliant, thanks!