asyncUtilWrap expects the next argument to be the last, which is incorrect when there are additional arguments

[robertbullen]

The unit test 'should provide additional arguments to the middleware' implies that arguments following the next callback are allowed. But asyncUtilWrap always looks for next as the last argument. Thus, in a scenario where additional arguments are provided and an error is thrown, next will not be properly called with the error.

[robertbullen]

Anyone?

[MunifTanjim]

Why would your express middleware arguments ever have anything after the next? The only two middleware signatures that express supports are:

• (req, res, next)
• (err, req, res, next)

Where are you even using this library?

[robertbullen]

I personally am not using a middleware signature with arguments after next(). But your question is very relevant.

There two pieces of evidence that suggest it is a supported scenario:

1. There are unit tests for it in this project.
2. Express's app.param() function takes a callback accepting an argument after next() (example copied from that API's documentation):

app.param('user', function(req, res, next, id) {

  // try to get the user details from the User model and attach it to the request object
  User.find(id, function(err, user) {
    if (err) {
      next(err);
    } else if (user) {
      req.user = user;
      next();
    } else {
      next(new Error('failed to load user'));
    }
  });
});

[MunifTanjim]

As far as I know, express-async-handler is only for using with express middlewares.

There are unit tests for it in this project.

I noticed that too 🤔 It was added by one of the contributors, but I don't think it was intentional (a208f5f)

Express's app.param() function takes a callback accepting an argument

Well, we better not throw in that callback 😅

[artursudnik]

Why would your express middleware arguments ever have anything after the next? The only two middleware signatures that express supports are:

• (req, res, next)
• (err, req, res, next)

Where are you even using this library?

I've just faced this issue when using this function for middleware in a parameter handler. The parameter name is the last argument then.

[agconti]

@robertbullen here's a fix to correctly get the next argument:

exports.handleAsyncErrors = fn => function AsyncErrorsWrapper(...args) {
  // next is always the last function of the arguments passed to an express middleware or view
  const getNext = args => [...args].reverse().find(arg => typeof arg === 'function')
  const next = getNext(args)
  return fn(...args).catch(next)
}

[dko-slapdash]

@robertbullen @artursudnik Hi from 2021! :-)

If there are extra parameters, I wonder how Express distinguishes e.g. (err, req, res, next) case from (req, res, next, myParam) case. It internally looks at handler.length and, if it's 4, then it realizes it's an error-catching handler and calls them. But with the parameters, how does it work?

[dko-slapdash]

BTW, it's worth assigning asyncUtilWrap.length to fn.length (via Object.defineProperty()) to forward the number of arguments to Express. It would then allow to use express-async-handler for error handlers too.

[artursudnik]

@robertbullen @artursudnik Hi from 2021! :-)

If there are extra parameters, I wonder how Express distinguishes e.g. (err, req, res, next) case from (req, res, next, myParam) case. It internally looks at handler.length and, if it's 4, then it realizes it's an error-catching handler and calls them. But with the parameters, how does it work?

Apparently, it does not have to. (req, res, next, id) is registered with app.param not app.use.