-
-
Notifications
You must be signed in to change notification settings - Fork 104
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
Comments
Here are the lines in question for reference: Lines 180 to 183 in d3a598d
|
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. |
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. |
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.
The text was updated successfully, but these errors were encountered: