Middleware refactor & remove Session shared state #120
Conversation
This allows a middleware to have access to application-specific data
| @@ -242,7 +250,7 @@ extension Application: ServerDriverDelegate { | |||
|
|
|||
| // Loop through middlewares in order | |||
| for middleware in self.middleware { | |||
| handler = middleware.handle(handler) | |||
| handler = middleware.handle(forApplication: self, handler: handler) | |||
There was a problem hiding this comment.
Seems like the indentation got weird here, can we align it so it indents past for.
(Might just be Github)
|
@tannernelson @loganwright Updated! |
| @@ -16,6 +16,6 @@ public protocol Middleware { | |||
| Call `handler(request)` somewhere inside your custom | |||
| handler to get the `Response` object. | |||
| */ | |||
| static func handle(handler: Request.Handler) -> Request.Handler | |||
| static func handle(handler: Request.Handler, for application: Application) -> Request.Handler | |||
There was a problem hiding this comment.
Why for here and not application? Is this recommended by the Swift 3 evolution guidelines? (Not a criticism, genuinely curious)
There was a problem hiding this comment.
Yeah, this is more in line with the latest swift API Design guidelines. The type is suppose to give more insight under them, so we're not expected to declare its an application.
There was a problem hiding this comment.
Something about this seems off to me...idk why.
I'm wondering if something like this would be better
class Middleware {
let app: Application
init(app: Application) {
self.app = app
}
func handle(handler: Request.Handler) -> Request.Handler {
return handler
}
}Thoughts?
There was a problem hiding this comment.
I feel like it works well as it is.
The one thing that may be offputting here is that the handler is a function that isn't the last argument?
If we did want to push for an instance I'd like to keep it a protocol with an init requirement.
There was a problem hiding this comment.
Keeping it static allows us to add and remove middleware while the application is running as well which could be useful.
And maybe you're right about the closure not being the last param...though we are only ever calling the middleware internally and the closure is usually be passed around as a variable. So it's not a huge deal.
Middleware refactor & remove Session shared state
@tannernelson This includes
Applicationin theMiddlewarehandlemethod, allowing theSessionMiddlewareto operate on application-level data and configure the session there, instead of it being a shared class variable, as we discussed.The spacing in the diff seems a bit off; It looks right in Xcode. Not sure what to do about that =\