Router refactor #307
Router refactor #307
Conversation
✅ Deploy Preview for robyn canceled.
|
|
@AntoineRR , great work. I will try to review this PR by today evening 😄 |
|
|
||
| /// Checks if the functions is an async function | ||
| /// Inserts them in the router according to their nature(CoRoutine/SyncFunction) | ||
| impl Router<String, Method> for ConstRouter { |
There was a problem hiding this comment.
Hey @AntoineRR ,
I am a bit unfamiliar with traits in rust. Can you please explain this statement to me? And the benefits associated with it?
There was a problem hiding this comment.
Sure !
About traits
Traits are a way in Rust to share behavior between structs. There is no inheritance in Rust as in a truly object oriented language like C++, so traits are more similar to interfaces in Java for example.
Here, defining the Router trait allows to specify what methods should be in a router : add_route and get_route. For every router we want to define, we have to implement this trait using the line above. This will force us to provide a definition for the add_route and get_route method that is specific to our struct, which is ConstRouter here.
About the generics
I didn't want to change the code too much so I kept the same return types and parameter types in every router struct. The return type of the get_route method is different across the routers (DynRouter and MiddlewareRouter return an Option<((PyFunction, u8), HashMap<String, String>)> while ConstRouter return an Option<String>), so I used a generic type to unify their behavior in the trait. Similarly, the route_type parameter of the get_route method can either be a MiddlewareRoute or a Method. I needed two different generic types to handle those differences: this is T and U in the definition of the trait : pub trait Router<T, U>. For ConstRouter, T is String and U is Method.
Benefits of using traits
With this PR, DynRouter, ConstRouter and MiddlewareRouter all implement the Router trait (I wanted to include the WebSocketRouter too but it seems too different from the others). This means the compiler is aware those share some behavior. We could now do some cool things like:
- Creating a
Vec<dyn Router>containing the routers - Create generic methods that take a type implementing the
Routertrait as parameter ->fn method<T: Router>(router: T) - Eventually provide a default implementation for the
add_routeandget_routemethods if we can make it the same for all structs (which I may try to do later) - If a new method needs to be added for every router, its implementation can be in the
Routertrait instead of in each struct - Maybe other things I forgot?
I hope this makes things clearer! In this PR I didn't take advantage of the trait for now, I wanted to do small iterations on the code instead of a really big one. I will try to improve things further if I have some time 😉
There was a problem hiding this comment.
Thank you @AntoineRR . This an excellent explanation 😄
There was a problem hiding this comment.
You're welcome ! 😉
There was a problem hiding this comment.
Thanks for the PR @AntoineRR :D . I have a few inline comments.
|
Ok thanks! I'll solve the eventual merge conflicts once the other PR is merged |
|
Looks like the other PR is gonna take some time. Great job on this one! |
Description
This PR refactors the
routersmodule of the Rust part of Robyn.I noticed some improvements could be made there so here is my take on it.
Noticeable changes:
RoutertoDynRouterRoutertrait to unify the behavior ofConstRouter,DynRouter, andMiddlewareRouterget_relevant_mapmethods (mapping is directly done at structure creation)I tried to keep the same logic as before. I think this could pave the way for further performance improvements for the router 🙂