Handle sync errors in the middleware

[typeofweb]

Currently synchronous errors are not handled by the middleware. I wasn't sure if it was by design or an oversight. When fn threw synchronously, asyncUtilWrap wouldn't return a Promise at all.

It's fixed in this PR. The fix uses the idea from the Promise.try draft proposal.

It's a breaking change.

[mhamann]

@abazhenov thoughts on merging this?

[wookieb]

Sync errors are handled by regular express error handling which I believe every "sane" application has.

const express = require('express');
const asyncHandler = require('express-async-handler');

const app = express();

app.use('/error', (req, res) => {
    throw new Error('test');
});

app.use('/async-error', asyncHandler(() => {
    throw new Error('test');
}));

app.use((err, req, res, next) => {
    // caught!
    res.status(500)
        .json({error: err.message});
});

app.listen(5000);

[christiaanwesterbeek]

@wookieb Are you making an argument not to merge this PR? Please elaborate.

[wookieb]

As stated above. async-handler does not catch sync errors but either way they're properly handled by error handler from expres. What's the purpose of wrapping the exception in a promise then?

[typeofweb]

@wookieb to make it more consistent and handle all kind of errors in a single place. This way you don't have to wonder if a function is synchronous or asynchronous, it's an implementation detail which is mostly irrelevant. The flow should be the same.

@Abazhenov I updated the PR to fix conflicts and it's ready to be merged now again.

[wookieb]

and handle all kind of errors in a single place.

Which is standard error handler for both cases and this is how it works right now.

This way you don't have to wonder if a function is synchronous or asynchronous

And I don't since both kinds of function work properly.

[christiaanwesterbeek]

I think @wookieb makes a valid point. Not sure if it invalidates this PR. But this library is about error handling in express routes. This library changes the usual way (call next(error)) of doing that. That alone asks for a new PR with a README.md that describes that.

[typeofweb]

@christiaanwesterbeek that's exactly my understanding of the purpose of this library. To unify all error handlers and handle errors in a single place, regardless of whether the source of error is a synchronous function or an async function. Similarly to what HapiJS 17 did.

@wookieb but this way you'll have to have duplicate error-handlers (separately for sync and async errors). Also, changing an implementation detail all of a sudden changes the whole errors-flow in your apps (eg. changing function to async function). That's really fragile and should not be happening. Hence this PR.

[Abazhenov]

I think that I am inclined to agree with @wookieb. I don't think there is an expectation of an async error handler to also handle synchronous errors.

[wookieb]

@wookieb but this way you'll have to have duplicate error-handlers (separately for sync and async errors).

@mmiszy Can you please tell me where is duplication of error handling in the example I gave?

[typeofweb]

@Abazhenov What I meant is that, when using this library, I assume that all my handlers return a Promise. It makes thinking of the code easier, and writing middleware simpler.

It is a true assumption for anything except for synchronous errors. My PR makes sure this is the case for any kind of code. Maybe this is out of scope of the library, I hoped not.

[christiaanwesterbeek]

Best way to settle expectations and assumptions is to add this to README.md

## Differences

### Async error handling without express-async-handler ...

### Async error handling with express-async-handler ...

### Sync error handling without express-async-handler ...

### Sync error handling with express-async-handler ...

[MunifTanjim]

This PR makes no sense. The library currently does exactly what is should do.

app.use('/throw', asyncHandler((req, res, next) => {
    throw 'error';
}))

app.use('/async-throw', asyncHandler(async (req, res, next) => {
    throw 'error';
}))

app.use((err, req, res, next) => {
  // err === 'error'
});

With both sync and async functions, the thrown error is passed to the error handling middleware. And it's all that this library aims to do.