Treat returned promise like calling next

[wesleytodd]

Ok, so I think this is the next evolution in promise handling support. We have error support (#47), but this would make it so a resolved promise returned from a middleware would be like calling next. For example:

app.use(async (req, res) => {
  res.locals.user = await loadUser()
})

There was a ton of conversation about making it "koa" like in #32. And like what was concluded in there, that is a large paradigm shift for this router. So instead of doing all that in one go, I figured it was better to just start with this smaller part.

I don't know what @blakeembrey thinks, but I think this is the best successor to his PR without going the full distance to handle await next() style usage. Thoughts?

[blakeembrey]

@wesleytodd Let me know if I've jumped the gun here, but I thought it would always be a good idea to avoid this behavior. How would you express that you are returning and not continuing through the stack?

My concern is that there are many use-cases where proceeding in the stack after a return is very bad. E.g. bad authentication. If you use the resolution of an async function here it's impossible to know if you stopped or not.

[wesleytodd]

Yep, this is why adding this support is a difficult decision. Since this was really simple to achieve I figured it was better to open the PR to start the discussion than opening another issue.

I think if we added this, it has a bunch of edge cases that are poorly handled and I agree that avoiding it would be best. That being said, if we do not get at least this simple async function handling it will be a long term problem.

So, unless we as the express community want to build a whole new router paradigm which more natively supports promises, we will loose users.

[blakeembrey]

@wesleytodd That's true. At this point it's pretty tricky and probably a much larger discussion. I think going the Koa-like route is the most straight-forward and powerful approach. I actually spent a long time exploring ideal middleware patterns for HTTP and arrived at https://github.com/serviejs/servie (initially I used this pattern for a universal HTTP request library before I realized it worked for server-side and client-side). Using a unified signature of Request => Promise<Response> allows the same paradigm to translate across front-end and back-end. It just depends how far Express.js can flex here.

Many people using Express.js are used to the familiar (req, res, next) => void signature. You can keep the same signature but make next() Koa-like. Have req and res be the HTTP interfaces as always.

At some point it sounds like we need to investigate making req and res be proxies though, since supporting HTTP/2 is on the table. If you make these proxies, should it introduce incompatibility with the change or try to stay compatible with HTTP/1? At which point, can we open up a large discussion of where we can take Express.js (e.g. Servie on one hand with the transport layer completely abstracted away, or Koa on the other hand with proxies to HTTP/HTTP2/etc).

[wesleytodd]

At this point it's pretty tricky and probably a much larger discussion

Agreed. I brought up to @dougwilson that I think we should have another CTC meeting, and I think this should be a topic we discuss.

translate across front-end and back-end.

I have been using the current paradigm (req, res, next) on both server and browser for a while. Not sure why you think this paradigm works better on the browser side, but I would love to hear more.

I think going the Koa-like route is the most straight-forward and powerful approach.

keep the same signature but make next() Koa-like

I think this might be true in a async/await/promise world. Again, I think this would be a big paradigm change. It would mean that we have to change things and expectations around the order things execute in the express router.

Also, I think that pattern is ultimately only better for a small set of use cases, where as what is in this PR solves the other 95% of the promise cases. Which is why I think despite the rough edges, it is the best current option.

Also, if we did the koa approach, I think there is still a question of if the next promise is resolved on the middleware stack being done, or the response being finished. What does koa do for that?

At some point it sounds like we need to investigate making req and res be proxies though

I agree this is what we will have to do, but I am not sure if that will effect this change. Will it in some way I am not seeing right now? I think it will really just effect that long term discussion.

[wesleytodd]

How would you express that you are returning and not continuing through the stack?

On this I was thinking it could be something like calling an api specially added for this purpose. Something simple like return false could do it, although I am not sure that is the best way.

[dougwilson]

Wouldn't this just not work at all if you want to respond to a route? Because since all async functions return a promise, won't that cause every middleware and handler to just always continue on to the next, with the 404 just always triggering for example? This assumes people are using async await for this middleware and handlers, which is already happening in Express.js today and will just become more widespread over time (which is partly why the promise rejection was added).

[wesleytodd]

Yep, that is what @blakeembrey was referring to when he said "returning and not continuing through the stack". That is the same as an async function with a response. I didn't add it at first because I was unsure, but I was thinking something like resolving with false could be the api if we liked that.

[dougwilson]

So is the suggestion to take an existing middleware like

app.use(function (req, res, next) {
  if (precondition()) return next()
  res.end('a response')
})

and rewrite it to the following?

app.use(async (req, res) => {
  if (!precondition()) {
    res.end('a response')
    return false
  }
})

It's kind of difficult to really understand what the false return is doing there, though, don't you think? I know there are people who want consistent returns enabled in lint, so they'll probably add return true to the other branches as well.

I kind of feel like the false is similar to a flag function arguments, where it's difficult to really remember what the boolean is doing (for example in transferBalance(1000, false) -- what does false mean?).

I'm not really saying if this is a good idea or not right now, as I think there is a lot of discussion to work out here. It's certainly a paradigm shift from stop-by-default to continue-by-default.

[wesleytodd]

It's kind of difficult to really understand what the false return is doing there

false is similar to a flag function arguments

I agree fully. I cannot think of an interface which is more fluent, so I went with one that is familiar. Like return false used in browser side events of old. We could use the "magic string" approach like next('router'), which has it's own problems. I would love to hear peoples proposals on a better api for this case.

It's certainly a paradigm shift from stop-by-default to continue-by-default.

Yes it is, and unfortunately it is just the paradigm promises forced upon us. With a promise interface you only get two signals, the resolve/reject status and the value resolved/rejected with. In this case we need an indicator for "resolved but also stop".

[dougwilson]

unfortunately it is just the paradigm promises forced upon us

Well, promises are just one type of flow mechanism in JS; they are not the only one, so not necessarily forced. For example, your initial example works in the 2.x version of router already if you add next(), so using async itself also doesn't really force the use of chaining them together for that flow.

This is not an argument to not use promise flow, just something I felt like I wanted to clarify, perhaps if it helps in thinking of alternative APIs.

In this case we need an indicator for "resolved but also stop".

What about returning the response object means to stop? It would fit existing inverse of the pattern I had above where you have return res.send(...) for example. Just a thought, though I have not tested / played with this more than writing it down just now, haha.

[wesleytodd]

if you add next()

"forced" may have been a bit strongly worded, but if developers expectations seem to be that a resolved promise is like calling next even though in this case it is not. So "forced" in this case I really mean, in alignment with developer expectations of how promise support should work. And I think developers who use promises do not expect to have to call next.

returning the response object

We could do this in conjunction with checking if the response was in the process of ending/flushing. That might meet most developers expectations, but it is similar to the boolean return in that it is less explicit and might catch some users unaware.

[dougwilson]

but it is similar to the boolean return in that it is less explicit and might catch some users unaware.

Yes, it has similar pitfalls as the Boolean solution, just was thinking of returning something different from a Boolean rather than an actual new idea :) Basically it's just res instead of false.

Depending on how long of a timeline we want to go down, if this can be kicked down the road until after the idea of just dropping the Node.js request/response objects altogether, it could be made such that the response object is no longer going to do something immediately and so to send the response, your promise chain of handlers needs to literally resolve into a response object to actually successfully send a response at all. This would also bring it closer to how some strongly-typed frameworks function.

It would basically at that point make promise handlers feel just the same as working with promises in any other way -- the response object that is resolved all the way up to the top is the response that will actually get sent -- nothing else.

Streaming would still work here, as it would be more similar to how koa you set ctx.body to a stream where your response object has a stream for the body and that response object getting resolved all the way up to the top will trigger reading from the stream to begin since it's now confirmed to be what is going out.

I think something like that would resolve a lot of the pitfalls here trying to wedge this type of paradigm into the Node.js response / response objects, which were made in an era before promises.

[wesleytodd]

@pillarjs/express-tc Sorry for the tag on this, but I am trying to clean thing up and get a clear understanding of what we would want to land for either the next v5 express prerelease or even generally router@2/express@5. What do we think of this? From a user experience perspective I really do still feel like this is what users want and expect from other ecosystems.

If we think this is a good idea I can try to address all the comments in here, but since it is long I didn't want to waste time on this unless it was clear folks agreed something to solve this should land if we can work out the details.

[blakeembrey]

Wow, this was a while ago. I still think we shouldn't do it. Too many foot guns and incompatibilities with existing sync behavior. For example, currently you can define many overlapping routes have "the first non-next calling one" handle it. This changes it so async functions behave the opposite. I think that'll produce a bunch of foot guns.

It also makes it impossible to do other things of interest, such as middleware that can handle a "before"/"after" using a next() => Promise<void> signature.