-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add routing service #187
Add routing service #187
Conversation
haha, we both created routing services at identical times... let the battle commence! #185 Let's review eachother's code and discuss design :) Edit: from a quick review it looks like your Service is much more "batteries included" and mine is much more "sparse simplicity". This is good! We should determine what is actually useful. I also notice you included History fiddling, which I feel doesn't belong in a Service (but I can absolutely be wrong). I opened #186 to discuss. I'll look more at your code to see if I missed something. |
@vitiral @hgzimmerman It's amazing thing! There was no one router and there are two now 😲 Thank you, friends! 👍 Let's try to join the best of two ideas. I could review the both on this weekends. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I am comming to a design which is very similar to yours other than the points I laid out. Comments appreciated!
/// A subset of the url crate's Url object that can be passed | ||
/// to crate consumers to deal with routing. | ||
#[derive(Clone, PartialEq, Debug)] | ||
pub struct RouteInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I strongly prefer that this be only two items:
state: Option<Value>
location: Location
The Location
type is then just the resolved items in:
js!{return window.location}
- OR
js!{return new URL(@{href.to_string()})}
i.e. we will handle any thrown SecurityError
s and panic/log there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main benefit is that we do not require an extra library, but also we are standards compliant with browsers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree that RouteInfo
should require holding onto a Location
struct. I think that RouteInfo
should only contain information essential to routing, which excludes information in the origin. This implies that it should contain
path: String
orpath_segments: Vec<String>
query: Option<String>
fragment: Option<String>
state: Option<T: JsSerialize>
I do agree that the Location API, once StdWeb adds support for it, should be used to get info about the current route during the handling of a PopStateEvent
. I think my current approach is misguided in storing the route string in the history's state, and then parsing that into a RouteInfo
. Instead, when the event is fired, I should get the RouteInfo from the location API (currently by parsing the href via url
, in the future, by using stdweb's Location
's fields), and try to attach the state if it was Some
.
I still do want to use the url
crate because my routing implementation uses it to validate routes passed to it by crate users, and I trust it to perform better than anything I could ever write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, my push_state
ended up looking like this:
/// Set the state of the history, including the url.
///
/// This will only trigger a state change in the frontend if `emit == false`
pub fn push_state(&self, emit: bool, state: Value, title: &str, url: Option<&str>) {
let route_fn = match self.route_fn {
Some(r) => r,
None => panic!("Attempted to set_state without initializing router"),
};
let url = match url {
Some(url) => url.to_string(),
None => self.window.location().unwrap().href().unwrap(),
};
self.history.push_state(state.clone(), title, Some(&url));
if update {
let info = RouteInfo {
location: parse_location(&url),
state: state,
};
self.callback.as_ref().unwrap().emit(info)
}
}
That parse_location
basically just calls js!{ return new Url(url); }
.
So... I ended up parsing the url anyway! I'm starting to think that doing it through a library is probably worth it.
This brings up an interesting point... none of what we are doing here needs to be in the yew
crate itself. We could make our own yew_route
crate! This would allow us to experiment and include whatever dependencies we might want.
Conclusion
I'm coming around to doing the url parsing in rust. I also agree that having type safety on the URL types (via a macro in the future) is worth pursuing.
However, I disagree with your layout. Why wouldn't RouteInfo
just be two fields, state
and url
, where url
is the Url
type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I want to make sure we support is the ability to set_state
to a partial URL, i.e. just the hash/query.
Another possibility is to require the user to hold onto their initial Url
"base" and use things like set_query
to set the query they want, etc. I think this is actually much more clean, although possibly at the cost of some ergonomics (the user has to have access to their base url at all relevant times).
Setting the query based on a base-url is currently annoying, so I opened servo/rust-url#447
src/services/route.rs
Outdated
pub fn new() -> RouteService { | ||
RouteService { | ||
history: window().history(), | ||
location: window().location().unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree 100% that we should unwrap
here.
However, I think we should resolve the attributes by unwrapping them as well into a plain struct (panic for SecurityError
).
src/services/route.rs
Outdated
/// The callback takes a string, parses it into a url, and then uses the result of that | ||
/// to create a message that the component will use to update itself with. | ||
// TODO I would like to include this in register_router | ||
pub fn create_routing_callback<COMP, CTX>(context: &mut Env<CTX, COMP>) -> Callback<RouteResult> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be removed. We are just calling context.send_back
, which is required for all services. I don't think this adds anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also disagree with the API of requiring the user to implement COMP::Msg::from(route_result)
. The callback can handle that by itself if we used it in the "normal" way: the user created the Callback
using a function of type Fn(RouteInfo) -> Msg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inclined to agree. I wanted the code within create_routing_callback()
to live within register_router()
but because I couldn't both mutably borrow the context to get access to the router in order to call that function, as well as pass a mutable reference to the context as a parameter to that same function, I ended up creating this awkward function that needs to be called beforehand.
Because everything else requires calling context.send_back()
, I don't see the problem with removal of this.
I personally like using the From
and Into
traits to structure my code, but I can understand that for some simple apps that only have static routes, it makes sense to just define all routing in one place, and the most logical place to do that is in the callback.
/// Registers the router. | ||
/// There can only be one router. | ||
/// The component in which it is set up will be the source from which routing logic emanates. | ||
pub fn register_router(&mut self, callback: Callback<RouteResult>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I chose to name this initialize
since it fits with yew::initialize()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initialize()
works, but I chose register_router()
because this isn't called at app start.
Conceptually, someone could write an app that has a bunch of possible states where the component that performs routing isn't even instantiated until they perform some task. This scenario may be inadvisable, but Yew currently allows for it. Because of this, the router component could be instantiated and destroyed multiple times.
I feel like there is a significant functional difference between yew::initialize()
and how the router hooks into a component to justify a different name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the RoutingService
is attached to the Model
by convention instead of the Env
then we could accomplish everything within a single new()
in the Model::create
method. Then we wouldn't have these annoying Option
types.
It also allows people to call set_state
(or whatever) in the Model::view()
method, which allows for buttons, etc to respond directly. Considering that you should not call Callback
s within update
I think this is practically a requirement.
What do you think?
src/services/route.rs
Outdated
|
||
/// Sets the route via the history api. | ||
/// This does not by itself make any changes to Yew's state. | ||
fn set_route(&mut self, route_info: RouteInfo) -> RouteDidChange { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is where things get interesting.
The javascript form of this history.pushState
which accepts:
- the
state
object (arbitrary javascript value) - a "title", not currently used
- a URL.
I think we should do the same. Requiring someone to construct route_info
is unergonomic IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The set_route
method is actually the main point of difference between our APIs, and I am convinced mine is better.
In my implementation it returns COMP::Msg
. The reason it does this is to allow the user to decide whether to propagate the "route change event". For instance, if you did set_route
in response to a button click you could choose whether to send the resulting message or send a different one. That is very intuitive IMO.
To do this all it has to do is call the wrapped Fn
in the context, which we need #188 for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To address your first comment regarding the ergonomics, yes, creating RouteInfo structs is a burden placed upon the crate user, but the alternative is to pass strings around, which can lead to bugs at runtime. For example, the function responsible for mapping route strings to component properties could recieve a string that wasn't correctly formatted because the developer concatenated "/parent/"
and /child
together to create /parent//child
and tried to use it for routing, when their mapping function expects /parent/child
. By forcing users to parse their strings into RouteInfo, these errors can be prevented.
In the spirit of Rust, I'm trying to make what could otherwise be a runtime error into a compile time error. I don't want the route service's functions to be "smart" and insert extra leading '/'
s or convert occurrences of //
to /
. I want those error cases to either not be possible, or throw compile time errors, or throw runtime errors the crate user could choose to handle as they wish, before routing and history/url manipulation even occurs.
Currently I have a pattern like:
Route::DogForum => RouteInfo::parse("/dog").unwrap(),
which, yes, could throw a runtime error.
I intend to move this to something like:
Route::DogForum => route_info!("/dog"),
which would invoke the RouteInfo's parse function at compile time, and stop compilation if the parser couldn't convert it to a RouteInfo. This would imply that the parse function used in the macro could only take &'static str
.
It would also look like the following for when the user wants to switch routes.
context.routing.call_link(route_info!("/parent/child"));
If you wanted to ensure that you only call currently routable links and defend against the possibility of the route strings to specific components changing and all "links" to them no longer referring to that component:
context.routing.call_link(component_1::Route::Parent.into() + component_2::Route::Child.into());
or in the case of wanting to attach dynamic information, something like this could be used:
// parse_relaxed() could be used to parse strings that possibly don't have a leading slash.
match RouteInfo::parse_relaxed(some_user_specified_value.to_string()) {
Ok(dynamic_route_info) => {
let route_info = route_info!("parent/child") + dynamic_route_info;
context.routing.call_link(route_info)
}
Err(e) => eprintln!("Dynamic route could not be parsed")
}
I think this extra ceremony for creating navigable information provides safety benefits that outweigh the concerns about verbosity and ergonomics.
src/services/route.rs
Outdated
/// Sets the route via the history api. | ||
/// This does not by itself make any changes to Yew's state. | ||
fn set_route(&mut self, route_info: RouteInfo) -> RouteDidChange { | ||
if route_info != self.get_current_route_info() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the spec actually allows you to do this, mainly to update the state (I think)
The new URL can be any URL in the same origin as the current URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I store the current route string as the state. I don't know if this is standard practice, or if in other routers they store the route as well as (part of?) the model. Conceptually you could restore the routed-to model's data on a back event using a tuple of Pre-Edit: I wrote the above text before I came to the conclusion that I actually want the ability to store arbitrary state in the History API. It no longer applies.(String, Option<Model>)
where Model
implements Storable
, I decided pretty early on that I only wanted to work with Strings to keep my implementation simple.
On to addressing your comment:
I specifically did not want it to be possible to add more history entries to the same page you are already on. Just like how a plain HTML page won't reload the page when you click a link that points to that same page, I don't want webapps built using Yew to be able to add history entries by clicking a link that appears in a header 30 times in a row. If they did that without that conditional, they would have to press the browser's back button 30 times to go to a prior page on the site.
The principle of least surprise indicates that Yew should prevent consecutive duplicate entries to the History API.
For all of the original points of concern I brought up, I think I've gotten to a point where I'm comfortable with them. If something isn't inherently clear, it can be addressed in a guide of sorts (eg, best practices for using routing with the fetch service). What I'm still struggling with is the interface presented to the users of Yew. Currently I have the crate user implement This allows flexibility in the case of dynamic paths. For example if instead of the pub enum Route {
CatForum,
DogForum,
ForumsList
} instead it was: pub enum Route {
Forum(String),
ForumsList
} The crate user can choose to map a path to the forum like so: match path_segment {
"cat" => return Route::Forum("cat".to_string()),
"dog" => return Route::Forum("dog".to_string()),
_ => return Route::ForumsList
} or even more dynamically: if path_segment == "" {
return Route::ForumsList;
} else {
return Route::Forum(path_segment.to_string());
} The problem I have with this current approach is that it is a lot of work for the user to perform per routable component, and I think it can be made to be even less. @deniskolodin I would like your thoughts about the following proposal. Is this a direction worth moving in? Using a proc_macro, it should be possible to define attributes that allow an Enum to convert to and from a a path string. // === Forums.rs ===
#[derive(Routable)]
pub enum Route {
// Instructs the current segment to be stored in the String, as well as to resolve the forum::Route from further path segments
#[route(path = String)]
Forum(String, forum::Route),
#[route(path = "")]
ForumsList,
} By deriving Routable, it would specify that all enum variants require an attribute marking them so that it knows how convert them to and from path strings. In the case where you have a routing component that handles page not found errors it could look like: // === Main.rs ===
#[derive(Routable)]
enum Route {
#[route(path = "forums")]
Forums(forums::Route),
#[route(path = "login")]
Login,
// It will only route to this path if an error is encountered, but will set the url to "pagenotfound" if it does
#[route(error, path = "pagenotfound")]
PageNotFoundRoute
} If any route at any level of routing couldn't be resolved, an error could be propogated upwards through the path-resolving hierarchy until it encounters an enum variant that is tagged with Enums that derived Routable would have The concerns about this approach are that proc_macros require nightly for now and that excessive use of macros may bloat compile time. #[route(path = String)]
Forum(String, forum::Route), On the other hand, this doesn't require significant changes to my current design, this merely acts as sugar on top of the existing proposal. |
@hgzimmerman Thank you for this PR. It's inspiring. Routing is a hard to implement feature. Yew looks like React and Elm, but actually has other architecture/implementation, because I couldn't transfer usual approaches to Rust and had forced to reinvent it. I think we should move the same way with a routing. We should reinvent it again. Sounds strange, but I'll try to dream up: we could use |
@deniskolodin interesting... components as the primary endpoint of routes... I think this missses that there can be a lot of information within the route. I feel that a Or are you thinking of doing a component+msg... or something? I feel like adding complexity here is a mistake. Fortunately it has been proven that a "simple" router can easily be an external crate -- so by all means we can try multiple solutions (including one supported in core). Regardless I think it's a good idea to try out a lot of soutions. Please include me on the review! |
Thanks Denis. I've been dogfooding my ~2.5kloc Yew-based project with the routing implementation in this PR and I have a few observations:
I think that for any robust routing system, the mechanism that performs routing needs to operate on a tree (either defined at compile time, or dynamically). Defining how that tree is constructed and the rules for its resolution from a string are the responsibility of the router authors. With your suggestion that we attempt to reinvent routing, binding the routing logic to the component/model, the following is the best proposal I could come up with in a short period of time. First, I suggest we move away from defining routing enums that dictate which child component gets rendered by the routing component. This would allow Yew to dynamically create a decision tree where each node in the tree can decide for itself if it gets rendered or if its parent node moves on to try to match against the next possible component. In all, some of the magic bits would have to be ironed out, but I think it could look roughly like; struct ParentModel {
...
}
impl Component<Context> for ParentModel {
fn create(props: Self::Properties, context: &mut Env<Context, Self>) -> Self {
// Some macro magic may be required for this to work
context.routing.register_routes(vec![ChildModel::from_route, PageNotFoundModel::from_route]); // Ordering is significant
...
}
...
}
impl Renderable<Context, ParentModel> for ParentModel {
fn view(&self) -> Html<Context, Self> {
html!{
<YEW_ROUTER></YEW_ROUTER> // Yew would insert the resolved child here
}
}
}
////////////////////////////
#[derive(Routable)]
#[path = "child"] // Will only match if the path segment == "child"
pub struct ChildModel {
...
}
impl Component<Context> for ChildModel {
...
}
impl Renderable<Context, ChildModel> for ChildModel {
...
}
//////////////////////////////
pub struct Props {}
pub struct PageNotFoundModel {
...
}
impl Component<Context> for PageNotFoundModel {
...
}
impl Renderable<Context, PageNotFoundModel> for PageNotFoundModel {
...
}
impl Routable for PageNotFoundModel {
// The Option return type would indicate if the route matched or not.
// If it did match, then the props are passed to the create function
fn from_route(_route: RouteInfo) -> Option<Props> {
Ok(Props {})
}
fn to_route(&self) -> RouteInfo {
route_info!("/PageNotFound")
}
} With this approach, I am worried about the possibility of re-creating the whole hierarchy of routable components when any part of the route changes. I don't know much about Yew's internals, but maybe in the section of code that resolves the route, it could check if the component corresponding to the props returned from This also has the problem of tightly coupling the router to Yew via the proposed specific routing "html!" syntax, making extracting it to another crate next to impossible. |
I've been thinking about the proposed alternative from above for a few days and I think I came up with something better. To avoid some of the magic and tight coupling, as well as removing some ambiguity on how the accidental recreation problem can be avoided, I propose the following. The /// The component that implements this trait shouldn't correspond to any route itself,
/// but does have children that the router will need to resolve.
/// When the router determines which child to route to, set_child() will be called and
/// will update the child field of the Model. This should be easily deriveable in most cases.
/// There should only be one Model that implements this trait, as it will be the start of where routing occurs.
trait RoutableRoot {
fn set_child<T>(&mut self, T)
where T: RoutableChild + Component<??> + Renderable<??>;
}
/// This trait should be implemented or derived for Models that sit below the RoutableRoot,
/// but have children themselves.
trait RoutableNode {
// RoutePath is just an alias to string
fn from_route(route: RoutePath) -> Option<Props>;
fn to_route(&self) -> RoutePath;
fn set_child<T>(&mut self, T)
where T: Component<??> + Renderable<??>;
}
/// The router will want to be able to resolve the component that implements this,
/// but because it doesn't have any children, there doesn't need to be any set_child function.
trait RoutableLeaf {
// RouteLeaf is a struct that contains the last segment from a vec of path segments,
// as well as options of both fragment and query.
fn from_route(route: RouteLeaf) -> Option<Props>;
fn to_route(&self) -> RouteLeaf;
} The router service would break down the existing RouteInfo structs into component The ChildModel would remain similar to implementation shown in my prior comment. For the ParentModel, it would now look like: #[derive(RoutableRoot)]
struct<T> ParentModel
where T: RoutableChild // Marker trait applied to both RoutableNode and RoutableLeaf (?)
+ Component<??>
+ Renderable<??>
{
#[child]
child: T
}
impl Component<Context> for ParentModel {
fn create(props: Self::Properties, context: &mut Env<Context, Self>) -> Self {
// Some macro magic may be required for this to work
let routes = magic_macro!(ChildModel, PageNotFoundModel]); // Ordering is significant
// This now takes a reference to &mut self that it uses to create a callback that will
// execute set_child() when it determines a child route.
context.routing.register_routes(routes);
...
}
...
}
impl Renderable<Context, ParentModel> for ParentModel {
fn view(&self) -> Html<Context, Self> {
html!{
{self.child.view()}
}
}
} The router service, using information from No idea if this is possible to implement (hence the ??s in the type signatures for Component and Renderable), but I think its a novel approach worth exploring. @deniskolodin I would like some reassurance that this is the direction we would like to move routing with Yew towards before I start attempting to implement a routing backend that supports this sort of interface to the crate user. EDIT: trait Router {
fn set_child<T>(&mut self, T)
where T: Routable + Component<??> + Renderable<??>;
}
trait RoutableNode : Routable + Router; // Is this distinct trait even necessary anymore?
trait Routable {
fn from_route(route: RouteLeaf) -> Option<Props>;
fn to_route(&self) -> RouteLeaf;
} |
@hgzimmerman Thank you for these experiments. The last concept come closer to a perfect solution. For example: // Somewhere in the framework
trait Routable {
fn get_part(&self) -> String;
fn tune_from_part(&mut self, path: &str) -> Result<(), Its404>;
}
impl<CTX, COMP> Scope<CTX, COMP> {
pub fn mount_in_place(...) {
self.fit_to_url_path(); // Impl depends on `COMP: Renderable`
}
}
struct Its404; // Error
type UrlPart = String;
// In the app
enum Model {
Scene1,
Scene2,
}
trait Routable {
fn get_part(&self) -> UrlPart {
let part = match *self {
Model::Scene1 => "scene1",
Model::Scene2 => "scene2",
};
part.to_owned()
}
fn tune_from_part(&mut self, path: &str) -> Result<(), Its404> {
match path {
"scene1" => {
*self = Model::Scene1;
Ok(())
},
"scene1" => {
*self = Model::Scene1;
Ok(())
},
_ => {
Err(Its404)
},
};
}
} Child components also have to play this rules. |
I did try to implement my 2nd/3rd suggestion but did run into problems regarding creating new components and trying to infer what a component is depending based on the props. That was a bad idea. Provided the I do have some concerns about how this would work to users of Yew. I think the component itself needs to be aware of the changes the router performs. This would involve 3 components, the main component, the users component (which could also just show a list of users), and the individual user viewer component. So the router would want to create sets of enums like: So directly mutating the component shouldn't be an option. Instead, you either want to allow the trait Routable: Component<...> { // you can only implement Routable for things that already implement Component
fn get_part(&self) -> String;
fn tune_from_part(&mut self, path: &str) -> Result<Self::Properties, Its404>;
} This creates additional questions. How do we determine if the route really did change for this component, and not some child component in the case of In regards to error handling, perhaps a default method could be added to Routable to handle route errors. By default, it does nothing, maybe it logs an error to the console. But users can override that to set the scene/child for a component if they want. Then if it is possible, a mechanism could be developed to bubble the error up to parent components until one is encountered that does override the handle_route_error method. |
src/services/route.rs
Outdated
@@ -37,6 +69,35 @@ pub struct RouteInfo { | |||
pub fragment: Option<String> | |||
} | |||
|
|||
impl Iterator for RouteInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an iterator to RouteInfo. I would love it something like this could be used to only expose relevant path information to particular components.
So the main Model would get the first segment, and then its child would get the next, etc, and would only expose the query and fragment to the leafs in the tree of components.
tune_from_part()
would take a RouteSection
that is generated from a mutable RouteInfo that is shared among scopes and use that to determine its child instead of a &str
.
This would require that the callbacks that would call fit_to_url_path()
always fire in order. Such that the main Model
updates first, then its children, then so on. If this ever occurs out of order, then this iterator approach would not work, as the path segments would not necessarily correspond to the components expecting to match on them.
Now that I look at it, I think that there a a few ambiguities that I would like to clear up regarding what you want out of a routing service. I'm inclined to keep throwing ideas at the wall about how routing could be done, starting with how we think what interfaces should be presented to users, and working backwards from there to see if it is possible to implement. Once we settle on a design, I'll try implementing another prototype.
Going in an entirely different direction...
html!{
<div>
<div> Normal component things</div>
<Router: />
</div>
} And all the `<Router: /> does is choose what the nested component would be. Because the state of a Component and what its routed child is should be orthogonal, I think they should be separated if possible. |
I have several concerns about this approach. From the way I see it, a user of this approach is required to:
This is a lot of boilerplate which does not fit into the rest of the ecosystem (rendering IMO should only happen from the The approach I outlined uses a single function to accomplish the routing task and can handle all of the above concerns. If the user wants additional security they can use enums and macros to validate that their endpoints are valid html as much as they would like. I think the router should stay out of the user's way and give them maximum flexibility. This solution gives very little flexibility requires them to use the router for only rendering a view. |
/// Sets the route via the history api. | ||
/// If the route is not already set to the string corresponding to the provided RouteInfo, | ||
/// the history will be updated, and the routing callback will be invoked. | ||
pub fn set_route<T: Router>(&mut self, r: T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In response to your statement regarding this approach being too structured regarding the use of RouteInfo
, I present this change that could be applied to set_route()
and replace_url()
:
Instead of taking <T: Router>
, it could instead take <T: TryInto<RouteInfo>>
and TryFrom<String>
, TryFrom<&str>
, and From<Router>
could be implemented for RouteInfo
.
This would allow crate users to call set_route()
with either a string type or a RouteInfo
.
In the case of the conversion to RouteInfo
possibly failing, an error could be printed to the console to let developers know that they formatted their route incorrectly if they are using strings.
I will make the argument that in order to easily resolve which page or page hierarchy to display when a user enters the site with a route already specified in the URL, the routing framework requires that the developer specify ways for a page to both set the route, as well as determine how to render itself by using the route. If that can be accomplished in a single function, I will be impressed, but as far as I can tell, your proposed solution does not facilitate the setting of the route via Yew. Your example uses I acknowledge that my approach brings about significant baggage in the form of boilerplate, but in order to address the requirements of both setting and "serializing" state via a route string, I think it is necessary and justified. To address your other concerns:
|
…ute using a String instead of a Router
I will have more complete thoughts about it later, but I would like to admit that I think I was wrong to prefer that the For the current implementation, I think that the I've used this router to build what is currently a 5kloc app, and will continue to make changes to this branch to iron-out edge cases. Feedback and discussion are still appreciated. |
272: Multi-threading, concurrency, agents r=DenisKolodin a=DenisKolodin This is a series of bold experiments and I really love 💓 this PR. It makes this framework a **multi-threaded** (it's not a joke) and brings actors model everywhere. Now your yew frontend-apps will be more _Erlang_ or _Actix_ apps like 🚀 Also, I've removed a context. Completely! Components simplified. Now it's an actor which you could connect to and interact with messages. Other benefit is your components could interact each other #270 Since this PR will be merged the framework turned into multi-threaded concurrency-friendly frontend framework. Sorry me for buzzwords overload ) It still need Routing #187 and fixes of the most issues. I'll get to that. But extra benefit of this PR: it fixes major emscripten issues #220 Remaining: - [x] Add CHANGELOG.md - [x] Update README.md - [x] Create issue: Send `Connected` notification for `Private` agents (#282) - [x] Create issue: Send `Connected` notification for `Public` agents (#282) - [x] Create issue: Implement `Global` kind of agents (based on `SharedWorker`) (#283) - [x] Create issue: Add components interaction example (#284) Co-authored-by: Denis Kolodin <deniskolodin@gmail.com>
I'm going to dive back into this in the coming days. I haven't played around with Yew 0.6 features (actor update) yet, but I think the recent changes map very well to what I want to accomplish with routing.
Hopefully, I'll have something like this ready in a couple of days. |
296: Routing service + Router Actor example r=DenisKolodin a=hgzimmerman Here is a new routing implementation to replace the one proposed in #187. It creates a bare-minimum routing service around which an opinionated Router Agent is constructed and then used in a new example. Co-authored-by: Henry Zimmerman <zimhen7@gmail.com>
Since was #296 merged, this can be closed now. |
Hey, this is in no way ready to merge, but I've been working on implementing routing in my own app and figured I could touch up my implementation and possibly have it merged, or at the very least, provide inspiration for a better routing service.
I think the core idea is good, but the way crate consumers interact with the service could use some improvement. An example has been added to demonstrate the service.
My general thoughts on things that should be improved appear in
TODO
comments.These changes have been tested on Firefox 60 and Chromium 61 in Linux. The test application was built using cargo-web 0.5.0 with the asmjs target using my system installed emscripten. emcc's version is 1.37.16.
Things I would like to address:
expect()
andeprintln!()
to keep the interface to the crate's users simple, but I'm not sure if that is the right approach in all cases here.window().location()
returningNone
would indicate that the browser doesn't support that feature. How could this be exposed to users of the crate better than by just panic!()ing? ~~ Is aRoutingError
enum in order?~~Result<RouteInfo, RoutingError>
that is returned from the routing callback.By providing checks for strings that contain '/' for paths, I feel like I have over-complicated the way crate users specify what route they want to go to. I would like to relax, and come up with a better scheme for converting routes represented by enums into an acceptable path because currently, this doesn't work with routes with more than 2 path segments.I think I came up with a reasonable solution by usingRouteInfo
s at the interfaces of the route service.url
crate into Yew, bloating the JS/WASM file served to users,and currently leaksurl::ParseError
into Yew's exported types, which isn't acceptable.Given how we are only ever dealing with the path, query, and fragment, it may make sense to handleString
->RouteInfo
conversions ourselves and not use theurl
crate. Any errors resulting from parsing could fall under the aforementionedRoutingError
emun.url
crate, although the interface presented to the crate user allows that to change in the future.route_info!()
macro be a good idea to allows users to specify routes by running the parser at compile time and failing to compile if the provided &str is invalid?This would be way better than the current approach that involves bringing in every(Update: The current approach is very amenable to conversion to this macro, ideally only moving from run-time to compile-time errors)Route
enum needed to construct your desired path and putting them in a vector that is used to construct theRouteInfo
struct.