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

Add middleware promise support #2763

Closed
wants to merge 4 commits into from
Closed

Add middleware promise support #2763

wants to merge 4 commits into from

Conversation

calebmer
Copy link

I've recently been playing around with ES7 and I am really liking the async/await promise feature and want to use this in my middleware. However, express does not do a good job at supporting errors in promises. Take a look at this example:

app.use(async function (req, res) {
  var data = await api.request('/data');
  throw new Error('Uh oh!');
});

For those unfamiliar with async/await the above is equivalent to:

app.use(function (req, res) {
  return new Promise(function (resolve, reject) {
    api.request('/data')
    .then(function (data) {
      throw new Error('Uh oh!');
    })
    .catch(reject);
  });
});

Express should be able to catch asynchronous errors from promises just as it is able to catch synchronous errors and pass these to error handlers down the chain. That is what this pull request does to the Express library. If the middleware returns a promise object, an error handler will be attached. If the middleware does not return a promise, nothing happens.

Without this support engineers must do the following:

app.use(function (req, res, next) {
  (async function () {
    var data = await api.request('/data');
    throw new Error('Uh oh!');
  })()
  .then(function () { next(); })
  .catch(next);
});

I want to see Express evolve with JavaScript and not just be discarded. Support for promises will be a huge step in this direction.

If you are curious about the practicality of implementing this in the status quo:

  • Babel supports async/await behind a flag
  • CO is using promises for their generator coroutine support

Another opinion I had (which is not included in this pr) was eliminating the need for calling the next function entirely for promise users. For example, this makes more sense:

app.use(function (req, res) {
  return new Promise(function (resolve, reject) {
    resolve();
  });
});

Than this:

app.use(function (req, res, next) {
  return new Promise(function (resolve, reject) {
    next();
    resolve();
  });
});

When taken to async/await land you just have this:

app.use(async function (req, res) {
   // Automatically proceeds to the next middleware
});

Synchronous functions would still require next.

In my opinion eliminating next in favor of the promise resolve method is the correct course of action. To my knowledge next was only ever implemented to allow for operations to be performed asynchronously. But in a promise world, next would be a vestigial structure.

I removed this functionality from the pull request as I feel it warrants some debate. The rest of the pull request makes natural sense seeing where the future of JavaScript, Node, and some of your progressive users are going.

More information:

@dougwilson
Copy link
Contributor

Thank you so much, this sounds awesome! We have promise support in our 5.0 roadmap (available at #2237) so this is great :) I see this touches a lot of the router, and since we won't be merging this in until the next 5.0 alpha, it would be great if you could change the PR to target the 5.0 branch (https://github.com/strongloop/express/tree/5.0) as there are probably merge conflicts that would keep us from accepting the PR as-is.

@calebmer
Copy link
Author

No problem on branch switching. I can do that when I have access.

I like the ideas proposed for next in this issue. I could implement that spec too if you'd like. Would you rather a seperate pull request?

@calebmer
Copy link
Author

Also, am I correct in discerning the router has been moved entirely to this package? If so do you want to close this pull request and I can refactor for that project then resubmit?

@calebmer
Copy link
Author

Just wanted to confirm where I should be aiming this pull request as the 5.0 branch does not even include the router code that I modified.

@dougwilson
Copy link
Contributor

Hey! I meat to respond earlier, but forgot, sorry! Yes, all router code has been split out into https://github.com/pillarjs/router for other projects to re-use and just in general make it easier to integrate with Express as part of #2411

@calebmer
Copy link
Author

calebmer commented Oct 1, 2015

Do you mind if I do a larger refactor to clean up router code and add support for #2259?

@dougwilson
Copy link
Contributor

Hi @calebmer , the more code you change in one PR, the longer it will take for us to review and understand your PR. The smaller the chunks you can do, the better for ever getting it merged.

@calebmer
Copy link
Author

calebmer commented Oct 1, 2015

Moved to pillarjs/router#25

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

Successfully merging this pull request may close these issues.

2 participants