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

Global async wrapper - is this valid? #3748

Closed
adamgiacomelli opened this issue Sep 18, 2018 · 6 comments
Closed

Global async wrapper - is this valid? #3748

adamgiacomelli opened this issue Sep 18, 2018 · 6 comments
Labels

Comments

@adamgiacomelli
Copy link

adamgiacomelli commented Sep 18, 2018

The issue we had was how to wrap all of the routes inside a router with an async wrapper (i.e. to have a global try/catch block on all controller functions) and we came up with this:

const wrapper = fn => (req, res, next) => {
  Promise.resolve(fn(req, res, next)).catch(next);
};

const asyncMiddleware = (router) => {
  const routes = router;

  for (let f = 0; f < routes.stack.length; f += 1) {
    for (let fs = 0; fs < routes.stack[f].route.stack.length; fs += 1) {
      routes.stack[f].route.stack[fs].handle = wrapper(
        routes.stack[f].route.stack[fs].handle,
      );
    }
  }
  return routes;
};

module.exports = asyncMiddleware;

Then we use the "middleware" like so:

router.use('/example', AsyncMiddleware(exampleRouter));

And we have effectively wrapped all of the routes inside exampleRouter in a catch block.

I am wondering if this makes sense and/or if there is substantial problems to expect when express is upgraded/updated ... etc.

@niftylettuce
Copy link

niftylettuce commented Sep 18, 2018 via email

@wesleytodd
Copy link
Member

Also you can checkout the 5.0 branch or the 2.0 branch of the router: pillarjs/router#60

I believe this support has landed and is just waiting a few unrelated things to be in the next express release candidate.

@dougwilson
Copy link
Contributor

Correct, I'm getting the 4.x patch ready, then merging into 5.x along with the router promise/async support for a new 5.x release. I'll ping in here once that's actually published to npm.

But as for the original question, the "safest" method is to try and resist reaching into internals, even though we try not to break those. You can do this by using the router alpha (https://github.com/pillarjs/router/releases/tag/v2.0.0-alpha.1) directly and app.use(router).

@marcospgp
Copy link

marcospgp commented Oct 10, 2018

@adamgiacomelli your solution seems unnecessarily complex. Can't you just use the wrapper as middleware instead of looping through all of the routes in the router? As described in this blog post: https://medium.com/@Abazhenov/using-async-await-in-express-with-node-8-b8af872c0016

I think you just need the wrapper and not the asyncMiddleware part.

Edit: Also check out this package https://www.npmjs.com/package/express-async-errors

@dougwilson
Copy link
Contributor

In order to keep this from spiraling down some strange place, I'm going to lock the conversation.

The "safest" method is to try and resist reaching into internals, even though we try not to break those. You can do this by using the router alpha (https://github.com/pillarjs/router/releases/tag/v2.0.0-alpha.1) directly and app.use(router).

This will also help out in your own interest, as you'll help us test this new router implementation that will be the backbone of Express 5.0, and you won't need this "work arounds" any longer. The more testing prior there is, you get the standard two benefits (1) it is less buggy at launch and (2) you have the opportunity to influence the design before it's final.

@expressjs expressjs locked and limited conversation to collaborators Oct 11, 2018
@dougwilson
Copy link
Contributor

I'm going to close this issue now that Express.js 5.0.0-alpha.7 has been published which includes the initial support for Promises in the router. Middleware and handlers can now return promises and if the promise is rejected, next(err) will be called with err being the value of the rejection. The implementation is seeking feedback from real usage, and please open any feedback as a new issue, either in this issue tracker or in the router issue tracker.

I am currently working on writing up Express.js-specific documentation on this feature, but in the meantime, the documentation can be found in the router repository:

https://github.com/pillarjs/router/tree/v2.0.0-alpha.1#middleware

The function can optionally return a Promise object. If a Promise object is returned from the function, the router will attach an onRejected callback using .then. If the promise is rejected, next will be called with the rejected value, or an error if the value is falsy.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

5 participants