jeremydaly / lambda-api

Lightweight web framework for your serverless applications
https://serverless-api.com
MIT License
1.41k stars 125 forks source link

Missing async/await on ErrorHandlingMiddleware #195

Closed Hulle107 closed 1 year ago

Hulle107 commented 2 years ago

Hej love the package so far.

But have found that you do not really do promise for Error Handling Middleware the same way as you do other middleware. This is what I think could help in that regard.

Suggestion: index.js

383. // Execute error middleware
384. for (const err of this._errors) {
385.   if (response._state === 'done') break;
386.   // Promisify error middleware
387.   await new Promise(async (r) => {
388.     let rtn = await err(e, response._request, response, () => {
389.       r();
390.     });
391.     if (rtn) response.send(rtn); r();
392.   });
393. }
394. // end for

As seen you could implement it the same way as you do for other middleware. IF there is a reason for not do it this way I really wanna know.

warrickhill commented 2 years ago

We are having the same issue. @Hulle107 your code above fixed it for us, have you made a PR for it?

yidah commented 2 years ago

I also had the same error and spent considerable amount of time finding the solution. Thanks for your code @Hulle107. I hope this can be fixed soon.

warrickhill commented 2 years ago

Made a PR with @Hulle107 code

sean-hernon commented 2 years ago

There are actually two issues here -- one is that the asynchronous error handlers were not supported (fixed in PR with async / await addition).

The other is that the promise was only being resolved in the next function. Which leads to some unidiomatic code. This is the problem that I encountered, which I will illustrate here, for anyone that may be experiencing the same problem.

It's normal to be able to do the following in an error handling middleware -

(err, req, res, next) => {
    if (condition1) {
        return res.status(400).send('BAD REQUEST');
    }

    if (condition2) {
        return res.status(403).send('FORBIDDEN');
    }

    if (condition3) {
        return res.status(404).send('NOT FOUND');
    }

    //etc

    next();
}

In the above example, you return early when you send the response, instead of calling the next function, which is the usual flow for a middleware, but following this pattern the promise in index.js never resolves, without the addition of the final r();.

The workaround for anyone experiencing this, is that you cannot return early and must currently call the next function (pending the above PR being merged).