Add construct_router function #447
Conversation
| @@ -66,7 +66,7 @@ where | |||
|
|
|||
| /// Modify the router with a mutable closure. This is where one may apply middleware or other | |||
| /// layers to the Router. | |||
| pub fn mutate_router(mut self, mut alter: impl FnMut(Router) -> Router) -> Self { | |||
| pub fn mutate_router(mut self, alter: impl FnOnce(Router) -> Router) -> Self { | |||
There was a problem hiding this comment.
How is FnOnce less restrictive than FnMut? Not understanding, honestly.
There was a problem hiding this comment.
This allows you to use closures that can only be called once, such as ones that take ownership, which won't satisfy FnMut. FnMut by definition must satisfy FnOnce, so this does allow more closures/functions.
| /// <div class="warning"> | ||
| /// You must call `into_make_service_with_connect_info::<SocketAddr>()` before serving this | ||
| /// router for matchbox to work | ||
| /// </div> |
There was a problem hiding this comment.
This is a little concerning. It seems too easy to get wrong. Perhaps a flyway pattern could help us?
However, I'd rather pass the 2 routes ("/" and "/:path") as arguments to build or have some build_with(Router) method alternative.
There was a problem hiding this comment.
Flyway pattern?
Yeah I was hoping that there was some way of passing across the service that ensured this but couldn't find a way.
A build_with alternative sounds like a good idea though, I think it's best to have something more flexible than just passing routes.
There was a problem hiding this comment.
I've implemented a build_with alternative, and added a test for how it can be used too.
There was a problem hiding this comment.
I'll try testing this soon. Thanks!
There was a problem hiding this comment.
Bump for a review when you get a chance
RaoulHC
force-pushed
the
raoul/builder-additions
branch
from
b26824f to
2b783af
Compare
This allows you to modify the final router while ensuring that the connect info is still available to the signaling server.
RaoulHC
force-pushed
the
raoul/builder-additions
branch
from
2b783af to
18357c7
Compare
|
I've rebased this, @simbleau could you take a look? |
There was a problem hiding this comment.
Thanks. iirc this has been a heavily requested feature by some, and I think the implementation as it is now looks good.
Approving, and will merge soon, but I'll leave it open for a little while to let @simbleau have a final look if he wants to.
If you want to use a single server and have matchbox signalling at a specific route it's useful to be able to get the router directly and work with that. You can't use
mutate_routeras that occurs before the final router construction and you'll likely end up with conflicting routes. I don't think you can expose a constructedIntoMakeServiceWithConnectInfoas that can't be merged/nested into another router, it just has to be called at the root level before use.Open to other suggestions to how to do this, but this seems like the most straight forward and flexible way to do it.
I also loosened the traits for
mutate_routerwhich I didn't end up needing, but it is more restrictive than necessary.