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

Feature Request: return result of calling handler #51

Closed
snytkine opened this issue Nov 28, 2016 · 6 comments
Closed

Feature Request: return result of calling handler #51

snytkine opened this issue Nov 28, 2016 · 6 comments
Assignees

Comments

@snytkine
Copy link

The one shortcoming with express router is that it does not have a return statement for calling handler.
I wrote a custom request handler that returns a Promise instead of service response right away.
I really need a router that returns the result of call to handler.
I think it will not break anything if you just add return statement to Layer and to Router function.
That would be awesome.

@dougwilson
Copy link
Contributor

Wouldn't that conflict with us being able to add promise support to the router? How would a passthrough return work as soon as there is one async handler in the chain? Please provide a more detailed outline of what you're think of, how it would work with all the various use cases and the ecosystem at large, thanks!

@snytkine
Copy link
Author

Right now the router calls this function: (this function does not return anything)
layer.handle_request(req, res, next)
and layer.handle_request does not return anything

What if layer.handle_request were to return the result of calling the handler and the router function would also return the value that is returned from layer.handle_request?

Basically whatever the handler functions returns the router should just pass through the return
I understand that most common case for a handler is to just write response to the provided res object and don't return anything. This is probably the 99.99% of all use-cases.

I think the creator of original router library in express did not even think of a case that handler may actually return some value.

What I have is controller that has same signature as all controllers have (req, res) but it actually returns Promise of response right away.

I am just looking for a router for my project that would be able to handle this case. So far I was unable to find any. Only 2 small changes are required to your router library, just adding "return" word in 2 places. I will not break any existing functionality, right now it just returns undefined and it will be returning a value.

If you plan to add promise support to router, it will not prevent you from doing so. You can just wrap returned value of Promise.resolve(result) and return that... It will work for me just as well. But I think you should just pass through return value from controller and let the user decide what they want to do with it.

@snytkine
Copy link
Author

snytkine commented Nov 28, 2016

This is what I want to do:

var Router = require('router')

Instead of this:
var router = Router()
router.get('/', function (req, res) {
res.setHeader('Content-Type', 'text/plain; charset=utf-8')
res.end('Hello World!')
})

I want to do this:
var server = http.createServer(function(req, res) {
var myResponse = router(req, res, finalhandler(req, res));
myResponse.then(o => res.send(o.body)).catch(e => res.statusCode(500).send("Server-Error")
})
server.listen(3000)

This is the type of framework I am creating, where controllers always return promise of response (custom object that has responseCode, headers and body) and never directly write to res object.

@dougwilson
Copy link
Contributor

PR #32 sounds like it implements what you're asking for, so I'm going to close this as a duplicate. Let me know if you believe you're asking for something different than that PR.

@blakeembrey
Copy link
Member

@dougwilson Note that this request is effectively for "upstream" support, but proper upstream support is impossible in the current module. This is because every existing piece of middleware is not written to cater for return next() and only handles moving forward in the stack. That PR went part way to return next(), but it's only useful internally and any external middleware in the mix will break. This is because going forward and backward in the stack would diverge when one promise can resolve while next() is still moving forward (because return is omitted and most existing middleware uses callbacks). This sort of functionality would be best moved into a new module that can be mounted with the compatible Express.js signature, which is actually very trivial to do thanks to the middleware simplicity (so one can have upstream and downstream within a subset of their application). A new router could also support legacy non-upstream supporting middleware, but it's a bit of a mess as you need to make next a function that resolves a wrapper promise and it's easier just to keep that functionality here. Something like:

function useLegacy (middleware) {
  return function (req, res, next) {
    return new Promise((resolve, reject) => {
      middleware(req, res, (err) => err ? reject(err) : resolve(err))
    }).then(() => next())
  }
}

@blakeembrey
Copy link
Member

@snytkine Also, you might be interested in koa as it's pretty much that approach.

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

No branches or pull requests

3 participants