Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unhandledRejection when returning promises from routes #19

Closed
sbarry157 opened this issue Oct 3, 2019 · 4 comments · Fixed by #20

Comments

@sbarry157
Copy link

commented Oct 3, 2019

We would like to start using awilix-express moving forward, but promise rejections are not handled as we expected. Using express-promise-router, our pattern is this:

all routes return promises
When an error happens, throw immediately.
register an error handler (app.use) to inspect the error and return the appropriate response.

I noticed #15 relates to this issue.

But in looking at the code, I see that the error is re-thrown after calling next. I think this is causing the issue.

function asyncErrorWrapper(
  fn: (...args: any[]) => any
): (...args: any[]) => any {
  return function asyncWrappedMiddleware(
    req: Request,
    res: Response,
    next: NextFunction
  ) {
    const returnValue = fn(req, res, next)

    if (
      returnValue &&
      returnValue.catch &&
      typeof returnValue.catch === 'function'
    ) {
      return returnValue.catch((err: any) => {
        next(err)
        throw err // <-- Does this need to be here?
      })
    } else {
      return returnValue
    }
  }
}

Looking at express-promise-router, they don't re-throw in this situation:

// Call the route
var ret = handler.apply(null, args);

// If it doesn't return a promise, we exit.
if (!isPromise(ret)) {
    return;
}

// Since we got a promise, we handle calling next
Promise.resolve(ret).then(
    function(d) {
        if (d === 'next') {
            next();
        } else if (d === 'route') {
            next('route');
        }
    },
    function(err) {
        if (!err) {
            err = new Error('returned promise was rejected but did not have a reason');
        }
        next(err); // <-- no rethrow
    }
);

Maybe I'm not using the library correctly?

@jeffijoe

This comment has been minimized.

Copy link
Collaborator

commented Oct 3, 2019

I think you're right, it shouldn't rethrow it.

jeffijoe added a commit that referenced this issue Oct 3, 2019
Fixes #19
@jeffijoe jeffijoe closed this in #20 Oct 3, 2019
@jeffijoe

This comment has been minimized.

Copy link
Collaborator

commented Oct 3, 2019

@sbarry157 released as 3.0.0, I also bumped the supported node version range.

@sbarry157

This comment has been minimized.

Copy link
Author

commented Oct 3, 2019

@jeffijoe excellent. Thanks for the quick resolution :)

@jeffijoe

This comment has been minimized.

Copy link
Collaborator

commented Oct 3, 2019

No problem! I'd suggest using Koa though. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.