Clean-up and optimize router a little bit #1869
Conversation
…te is obtained This has the drawback of requiring the `Routable` enum to be `Clone`. Generally, this shouldn't be an issue as cloning the value *should* be cheaper than making a DOM API call and then running the route recognition process
|
@Diggsey, can you take a look? I implemented some of the optimizations from your PR in the current router. Also, sorry I didn't mention you earlier. I was going to but forgot |
|
Seems reasonable - it's maybe surprising that |
Those methods aren't really supposed to be used by the user. We provide abstractions over those which should be used instead. I feel like it is a documentation issue.
That's a good point. I will add a destroy method which is called from component's destroy and will clean up the state. |
|
Should probably update the documentation to state that |
| /// | ||
| /// # Usage | ||
| /// | ||
| /// The functions exposed by this trait are **not** supposed to be consumed directly. Instead use |
There was a problem hiding this comment.
I personally don't think it's great API design to have a derivable trait that isn't supposed to be consumed. The Routable trait could function as a useful abstraction outside of the Router component if it were a bit more flexible.
This is just a side note though, there's no need to do anything about it in this PR.
There was a problem hiding this comment.
It is generally the case that Routable won't be consumed directly by the user. The doc comment exists because macro introduces state and therefore care must be taken that a manual usage cleans up properly. See the above comment about side-effects.
| BASE_URL_LOADED.call_once(|| { | ||
| BASE_URL.with(|val| { | ||
| *val.borrow_mut() = fetch_base_url(); | ||
| }) | ||
| }); | ||
| BASE_URL.with(|it| it.borrow().as_ref().map(|it| it.to_string())) |
There was a problem hiding this comment.
Why not use an option instead of using both an additional once?
There was a problem hiding this comment.
fetch_base_url needs to called only once from this function at the very start to populate state. Option can't tell the difference between uninitialized and no route found. In fact, Option is used currently for the latter case.
| let route_listener = attach_route_listener(link.callback(Msg::UpdateRoute)); | ||
| let route_listener = EventListener::new(&yew::utils::window(), "popstate", move |_| { | ||
| link.send_message(Msg::ReRender) | ||
| }); |
There was a problem hiding this comment.
Why did you move away from the modular API again?
There was a problem hiding this comment.
Optimization. The API changed, meaning that in the previous implementation, it'd now read the current route twice. This change ensures the current route is checked and obtained once by the component.
There was a problem hiding this comment.
That's very unfortunate, are you sure there's no way to avoid that?
There was a problem hiding this comment.
There will be one when the listener is attached and called and one for the initial render. I'm not aware of any ways that provide a way to handle this. Iirc, the tests failed with the previous implementation (don't exactly remember though)
|
Visit the preview URL for this PR (updated for commit f86da84): https://yew-rs--pr1869-router-cleanup-jdmv9nlf.web.app (expires Mon, 07 Jun 2021 17:40:42 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
|
@siku2 I made the changes. Take a look. |
Description
This PR fixes the hack I introduced in #1859 and also adds a couple of optimizations optimizations.
Checklist
cargo make pr-flow