-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
service provider support #52
Conversation
…instead of instantiating them
44edb98
to
cbdb713
Compare
Request.Handler should return a Response, not a ResponseConvertible. I want to keep all Response conversion in the Router / Application. Use Route.Handler to refer to a request handler that returns a ResponseConvertible. |
@tannernelson updated to retain that functionality |
|
||
do { | ||
let convertible = try handler(request: request) | ||
return convertible.response() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not need to convert to .response()
now.
Oops sorry if I wasn't clear. I think the Route handler should be on the Route class. ResponseConvertibles are resolved to Response's before they are even registered to the Router. I'd rather keep the ResponseConvertible logic as isolated as possible so that middleware and routers etc can work with the simpler Request handler. |
Check the closed PR to see my implementation. |
@tannernelson In the new setup, we omit the need for a global Routes array which I think is preferable for users who may want to implement a custom Router since it's much more modular. By converting inside of the Router, we allow access to the simpler |
Oh, I see what's happened. The API for adding routes was moved to the app.get("test") { request in
return "test"
} I think this API is very clean. You register your This isolates |
If you copy the |
@tannernelson I see where we missed each other, updating now. |
@tannernelson Ok, the syntax has been returned to the application, I like this way much better since it omits the need for a singleton while still keeping interaction simple and clean. Good choice! It also works better for the router since we can rely on its registration component and leave modularity easy for swapping in a new |
} | ||
|
||
public final class Router { | ||
public final class Router: RouterDriver { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a more descriptive name we can give this router? Like BranchRouter
or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, definitely
This looks great. Do we still have support for hosts? app.host("google.com") {
app.get("test") { request in
return "this is only available at google.com/test"
}
} If so, let's just see if we can write a few tests to ensure middleware / providers are working and let's merge this. It will be release Also we should update all dependencies on vapor, like vapor-example, to specify |
Everything looks good. Did we add any new tests with this? |
Moving here from #48 w/ conflicts resolved. Can overwrite 48 if preferred.