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

Move Options Responder to Separate Module #44

Open
wesleytodd opened this issue Jun 28, 2016 · 3 comments
Open

Move Options Responder to Separate Module #44

wesleytodd opened this issue Jun 28, 2016 · 3 comments

Comments

@wesleytodd
Copy link
Member

To contine the conversation from #14:

Moving the options responder out into a separate module that is optionally included here seems like a good optimization to me. Also, seems like this is not the domain of that feature anyway.

This would be a breaking change, so probably couldn't land until a 2.0. But I think it would be a simple enough change. Then we could integrate that change into express, even 5.0, by making sure express implements the options handler in the same way it works now.

@wesleytodd
Copy link
Member Author

Here are the lines in question for reference:

router/index.js

Lines 180 to 183 in d3a598d

if (req.method === 'OPTIONS') {
methods = []
done = wrap(done, generateOptionsResponder(res, methods))
}

@eth0lo
Copy link

eth0lo commented Jun 29, 2016

Let's see, I think this is a grey area, let me explain, the router is responding to an options request based on the handlers defined to a given url; personally I think that is expected and it should be kept as part of the router.

Also in your last comment, you mentioned that this will never be used in the browser, well that actually depends; if you add router to be used as part of a service worker you can responde to browser requests.

Now my concern if the router should tell the transport, in this case the response object, the content length of it, and that's probably what it shouldn't be there IMHO.

@wesleytodd
Copy link
Member Author

wesleytodd commented Jun 29, 2016

Even with the service worker use case, the router is for routing. Let the layer above handle what requests to respond to and how. I also agree that the router should not be used to tell the transport anything, which is why I say we should move the whole thing out of the router.

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

2 participants