Middleware per handler #133 #149
Conversation
0michalsokolowski0
commented
Sep 30, 2019
0michalsokolowski0
commented
- Implemented Middleware per handler
- Added tests
message/router.go
Outdated
| // first added middlewares should be executed first (so should be at the top of call stack) | ||
| for i := len(middlewares) - 1; i >= 0; i-- { | ||
| middlewareHandler = middlewares[i](middlewareHandler) | ||
| for i := 0; i < len(middlewares); i++ { |
There was a problem hiding this comment.
so as we discussed - it should be done like before ;) (middlewares are applied in reversed order because the "outside" middleware is called first)
There was a problem hiding this comment.
Yes, you are right I will remove the change.
|
The new API can be added to the docs as well. :) Probably the best place is https://watermill.io/docs/messages-router/#middleware |
* add flag to recognize router level middleware * add new Handler type
message/router.go
Outdated
| @@ -397,19 +423,24 @@ type handler struct { | |||
| messagesCh <-chan *Message | |||
|
|
|||
| closeCh chan struct{} | |||
|
|
|||
| router *Router | |||
There was a problem hiding this comment.
Seems like this one is no longer needed.
message/router.go
Outdated
| handler *handler | ||
| } | ||
|
|
||
| func NewHandler(router *Router, handler *handler) *Handler { |
There was a problem hiding this comment.
it probably may be private ;)
There was a problem hiding this comment.
or even we can create it without constructor (it's used only in the package scope once, so it's fine)
message/router.go
Outdated
| IsRouterLevel bool | ||
| } | ||
|
|
||
| func newMiddleware(handler HandlerMiddleware, handlerName string, isRouterLevel bool) middleware { |
There was a problem hiding this comment.
here also - we can live without constructor ;)
message/router.go
Outdated
| r.addMiddleware("", true, m...) | ||
| } | ||
|
|
||
| func (r *Router) addMiddleware(handlerName string, isRouterLevel bool, m ...HandlerMiddleware) { |
There was a problem hiding this comment.
in general IMO it's a good idea to avoid any bool in arguments and create separated functions here (and as I understand we need to pass handlerName only if it's handler level middleware, so it may also simplify it a bit
but in general, I like the design to keep everything in one slice :)
|
@0michalsokolowski0 I added a couple of comments, but I like the current API, what do you think @m110? It would be also nice to add some documentation of how to use it :) (you need to find in the docs where it's already mentioned how to use middlewares) |
|
Yeah I think the API is looking good now, I would maybe consider if we can come up with better names than |
* add flag to recognize router level middleware * add new Handler type
…0/watermill into middleware-per-handler
|
@roblaszczak @m110 Please correct me if I am wrong but it seems to me that currently examples can only be updated after a release. So if I would like to extend _examples/basic/3-router I can only do it after a release, since the example's dependencies point to specific version not the current implementation. |
|
Yes, that's true. One way would be to add a draft PR, with go.mod pointing to this branch and after release just bump the version there. |
