Rewrite router more

[Diggsey]

Description

This implements the ideas I described here: #1791 (comment)

Core changes

• Instead of just considering the path alone, the path, query, hash and state values are bundled into a Route struct.
• A non-generic HistoryAgent is implemented which binds the browser's history API to a component via this Route struct. This can be used on its own without the higher level Routable API if desired.
• The Routable trait is now simply converts to/from a Route, and is implemented by Route.
• A new Router struct is implemented. This is not a component, it acts more like a Bridge to the HistoryAgent, but generic over a Routable type. It can be used from a functional component via the use_router hook, or from a classical component via Router::new.
• The Router acts as a two-way binding between the component and the current route: it allows accessing the current route, and it allows changing the current route.
• The two-way binding used by a Router can be overridden via the context mechanism, allowing an entire application to be "mounted" within another, without the inner application needing to be aware of this.

Example of using the router from a functional component:

#[function_component(Comp)]
fn component() -> Html {
    let router = use_router();
    let onclick = router.dispatcher(|_| {
        Some(RouterAction::Push(Routes::No {
            id: 2,
            foo: "bar".into(),
        }))
    });

    match &*router.route() {
        Routes::Home => html! {
            <>
                <div id="result">{"Home"}</div>
                <a onclick=onclick>{"click me"}</a>
            </>
        },
        Routes::No { id, foo } => html! { <No id=id foo=foo /> },
        Routes::NotFound => html! { <div id="result">{"404"}</div> },
    }
}

Derive macro changes

• The derive macro was redesigned to be more powerful (since it must now support binding to query parameters, hash values, state, etc. as well as the path).
• The new design is intended to be extensible.
• Replaced #[not_found] with just a Default implementation as it allows the user to specify specific field values.
• Added #[bind(rest)] binding mode, which means the route does a prefix match, and the rest of the route (including any unmatched query arguments, state, hash, etc.) are decoded into the corresponding field, which is itself a Routable type.
• Added #[bind(query_arg)], #[bind(hash_arg)], #[bind(pop_state)] modes, which allow extracting arguments from the query string, hash string, and state. More binding modes can easily be added (eg. #[bind(hash)] which could just extract the entire hash without attempting to decode it into individual arguments.

Example of deriving Routable:

#[derive(Routable, PartialEq, Clone, Debug)]
pub enum Route {
    #[at("/posts/:id")]
    Post { id: u64 },
    #[at("/posts")]
    Posts {
        #[bind(query_arg = "p")]
        page: u64,
    },
    #[at("/authors/:id")]
    Author { id: u64 },
    #[at("/authors")]
    Authors,
    #[at("/")]
    Home,
    #[at("/404")]
    NotFound,
}

General fixes

There are some issues with the current approach to routing: see https://github.com/yewstack/yew/blob/master/examples/router/src/pages/post_list.rs#L31 - because the current page is managed entirely within the component state, the component cannot react if the page number is changed directly. The "next page" button relies on being able to directly send this message.

(This is still very much a draft)

Checklist

• I have run cargo make pr-flow
• I have reviewed my own code
• I have added tests

[ranile]

I like the direction in which this is going but there a few things worth discussing:

1. I'm not sure if we should use agents for this. There is a function which does the job of adding a listener right now. Agents increase complexity too much and add a lot of boilerplate.
2. What's wrong with Router being a component? I prefer having a component which handles the routing and renders the UI over a struct which just implements route handling. How about we have both? Let's provide a Router component which uses the struct internally.
3. You add a dependency on regex. Let's try to avoid it. Regex parsing is kinda slow and doing it on main thread, especially for router is something to be avoided.
4. Should the router provide a way to handle state? Is it worth the complexity over using, say, yewdux?

I like the bind attribute for the derive macro and how query parameters are parsed. Maybe we can also use it for #1858.

CC: @siku2, thoughts on this?

[Diggsey]

Thanks for reviewing! Here are my thoughts on each of the points you raised:

1. I'm open to alternatives: there are two agents introduced here, so I'll list the justification I had for each:
   • HistoryAgent
     This manages subscriptions to the current state of the history. Being an agent means it only needs to install one set of event listeners regardless of how many components are subscribed to the history. It makes managng the subscriptions very easy. It means that building the Route type from the current URL only needs to occur once per URL change.
   • RouterAgent
     Similar arguments as above, in particular the conversion from Route -> [Routable] only needs to occur once, which is the most expensive part of the router.

Whilst agents can be more verbose to interact with, you don't need to interact with either of them directly to use the router. You would use the Router type directly which hides the details of the agent(s) anyway.

I think there are some options for improving the API though: for example, it might be nice to be able to initiate a router action without first constructing a Router if you don't care about the current route/responding to route changes.

2. The Router component just didn't really do anything anymore (it takes no arguments and calls a render function) so why not just have that be implicit? I felt the new setup dramatically reduced the amount of boilerplate, especially for functional components, since there's no need to define a separate render function.

3. I'm using regex for the RegexSet type, which allows directly jumping to the (potentially) matching route without having to try each in turn. If anything I would expect this to be one of the fastest possible ways to route. There is some initial overhead to compiling the regex, but beyond that it should be fast compared to other options.

   It's possible that a bespoke solution that generates the state-machine at compile-time could be faster, but I think it would be extremely hard to beat regex here.

I like the bind attribute for the derive macro and how query parameters are parsed. Maybe we can also use it for #1858.

That's already supported with #[bind(hash_arg)] :)

[ranile]

I'm open to alternatives: there are two agents introduced here, so I'll list the justification I had for each:

• HistoryAgent
  This manages subscriptions to the current state of the history. Being an agent means it only needs to install one set of event listeners regardless of how many components are subscribed to the history. It makes managng the subscriptions very easy. It means that building the Route type from the current URL only needs to occur once per URL change.

• RouterAgent
  Similar arguments as above, in particular the conversion from Route -> [Routable] only needs to occur once, which is the most expensive part of the router.

Why not put the state in the Router instance? I'd keep the state inside the instance and provide methods to work with it. Also, for communication, it would be easier since everything is in one object. It might be worth taking a look at futures crate. It provides a nice implementation for channels which works well on WASM. We can provide a router-specific wrapper around it, if necessary.

The Router component just didn't really do anything anymore (it takes no arguments and calls a render function) so why not just have that be implicit? I felt the new setup dramatically reduced the amount of boilerplate, especially for functional components, since there's no need to define a separate render function.

It wouldn't hurt to provide a component interface for Router

I'm using regex for the RegexSet type, which allows directly jumping to the (potentially) matching route without having to try each in turn. If anything I would expect this to be one of the fastest possible ways to route. There is some initial overhead to compiling the regex, but beyond that it should be fast compared to other options.
It's possible that a bespoke solution that generates the state-machine at compile-time could be faster, but I think it would be extremely hard to beat regex here.

I would benchmark both implementations to see the differences and then decide if we should keep using regex or not. Also, it might be faster to use DOM API of regex instead of a Rust implementation (not fully sure though).

That's already supported with #[bind(hash_arg)] :)

Great

[Diggsey]

Why not put the state in the Router instance? I'd keep the state inside the instance and provide methods to work with it. Also, for communication, it would be easier since everything is in one object. It might be worth taking a look at futures crate. It provides a nice implementation for channels which works well on WASM. We can provide a router-specific wrapper around it, if necessary.

There isn't just one Router though: a Router is just a handle to either a RouterAgent or parent Mount component, and any state it contains is specific to the component which is making that connection.

You said originally:

Agents increase complexity too much and add a lot of boilerplate.

Are you referring to complexity and boilerplate in user code, or complexity and boilerplate within the implementation of yew-router itself?

It wouldn't hurt to provide a component interface for Router

Sure.

I would benchmark both implementations to see the differences and then decide if we should keep using regex or not. Also, it might be faster to use DOM API of regex instead of a Rust implementation (not fully sure though).

AFAIK, there's no JS equivalent to RegexSet, and trying multiple regexes in turn would be considerably slower.

[ranile]

There isn't just one Router though: a Router is just a handle to either a RouterAgent or parent Mount component, and any state it contains is specific to the component which is making that connection.

I mean replacing RouterAgent or whatever else is there with the Router itself. Only one (not counting #1853 use case) would exist.

Are you referring to complexity and boilerplate in user code, or complexity and boilerplate within the implementation of yew-router itself?

User-facing code for the most part. The implementation also gets complex but that isn't a that big of a deal as long as there are good and concise abstractions over it.
Are the agents you added supposed to be user-facing?

AFAIK, there's no JS equivalent to RegexSet, and trying multiple regexes in turn would be considerably slower.

I would say, it's still worth benchmarking if adding regex improves performance, and by how much. This is an additional dependency which will also increase final binary size.

[Diggsey]

I mean replacing RouterAgent or whatever else is there with the Router itself. Only one (not counting #1853 use case) would exist.

Hmm, OK. (BTW, this PR does allow nested routers as per #1853, but there is still only one RouterAgent in that case: the nested routers connect to the parent Mount component via contexts, instead of an agent)

The HistoryAgent could potentially be implemented as a lazy static instead of an agent, although I'd be duplicating the subscription functionality of agents, so I'm still a little hazy on the benefit? To me agents seem like a pretty great way to bundle up state and allow multiple components to interact with it?

The only downside of agents I see is that you need to create a bridge before sending a message, even in cases where you don't need to receive messages from the agent. Is that the complexity you don't like?

Are the agents you added supposed to be user-facing?

They are currently public, but I don't expect users to need to use them directly, and would be happy to make them private. The HistoryAgent could potentially be useful if a user wanted to parse the Route in an entirely custom way, but it would be unusual.

I would say, it's still worth benchmarking if adding regex improves performance, and by how much. This is an additional dependency which will also increase final binary size.

Binary size and startup cost seem much more likely to be potential downsides to the regex approach.

[ranile]

The only downside of agents I see is that you need to create a bridge before sending a message, even in cases where you don't need to receive messages from the agent. Is that the complexity you don't like?

It is a part of it. There's also cost associated with creating a link to agents. Also, afaik, we need to use use_effect to use agents in function components. Not only the complexity from user's side but also implementation complexity is increased quite a bit.
Is there still a way to interface with router, from user's perspective, without using agents?

Also, the current API is pretty confusing. We construct a router but why do we have to pass a Callback? new function is conventionally used to construct a completely new instance. In this case, it doesn't. What was wrong with the previous API? I think if we should combine the positives of both implementations.
Here's what I think we should do:

From this implementation

• Keep the macro's external API.
• Keep support for query and hashes
• Mount component

From previous implementation (#1791)

• Keep the inner workings

• Keep external API of the router
  It was intentionally kept very simple to handle the router, without the need to construct any objects

  I would love to never see another "how get the current route" or "how to set route" question.

  - @​siku2 (from Rewrite router #1791 (comment))
  I highly agree with that.

I'm not really sure about what to do with state. It seems like it's pretty hard to handle it efficiently (without serde) without exposing JsValue (which really shouldn't be done)

Binary size and startup cost seem much more likely to be potential downsides to the regex approach.

Those are pretty major downsides if you ask me.

[Diggsey]

Also, afaik, we need to use use_effect to use agents in function components. Not only the complexity from user's side but also implementation complexity is increased quite a bit.
Is there still a way to interface with router, from user's perspective, without using agents?

I'm not sure I understand? From the user's perspective they just use use_router or Router::new depending on whether they are writing a functional component or not. They never even know that (or even if) the router is backed by an agent.

Also, the current API is pretty confusing. We construct a router but why do we have to pass a Callback?

The callback is used to respond to route changes. For classical components, that means triggering an update which is most easily done by sending a message from a callback. If you don't need to respond to route changes you can just use Callback::noop, but your Router instance won't see any changes to the current route since it was constructed.

With use_router you don't need to supply a callback since it hooks up that aspect itself, and automatically ensures the component is re-rendered.

What was wrong with the previous API?

First of all, it seems quite error-prone. If I call current_route() from a component, it won't get re-rendered when the route changes. I'd have to manually attach a listener via attach_route_listener, but then there's no way to detach it when the component is destroyed? Basically I'd have to manually do all the stuff that use_router just handles automatically.

Secondly, the Mount functionality cannot work with the old API, because those global functions don't have access to component context.

Thirdly, current_route() has to parse the current route on every call, whilst using an agent (at least internally) means you only need to do this when the route changes.

I would love to never see another "how get the current route" or "how to set route" question.

I agree with this too, but Rust is about correctness: if there is subtlety to a task, then it's better to make the user think about that subtlety. In this case the subtlety is "do you really want the route at the time your component was constructed, or do you actually want the current route at any given time.

Also, this is how:

let router = use_router();

// Get the current route
let current_route = router.route(); // Automatically updates when the route changes.

// Set the current route
router.push(new_route);

[ranile]

I'm not sure I understand? From the user's perspective they just use use_router or Router::new depending on whether they are writing a functional component or not. They never even know that (or even if) the router is backed by an agent.

I misunderstood. I thought agent was to be used the user.

If I call current_route() from a component, it won't get re-rendered when the route changes

Why should it? current_route() means I want the current route at the time of calling and that's exactly what it provides.

I'd have to manually attach a listener via attach_route_listener, but then there's no way to detach it when the component is destroyed

It gets detached automatically when the handle is destroyed. Internally, the handle is just an opaque wrapper to hide the the gloo's EventListener. When EventListener is dropped, the listener gets detached. This is a documentation issue (see: #1852)

Secondly, the Mount functionality cannot work with the old API, because those global functions don't have access to component context.

I'm not sure what Mount provides. Can you explain that?

Thirdly, current_route() has to parse the current route on every call, whilst using an agent (at least internally) means you only need to do this when the route changes.

That could be easily fixed by adding a thread_local. That solution wouldn't be complex implementation wise and would do the job well.

In this case the subtlety is "do you really want the route at the time your component was constructed, or do you actually want the current route at any given time.

current_route gives you the current route at time of calling, attach_route_listener notifies you when a route is changed and gives you the new route. render callback takes in the current route and should pass down the data as props so children are re-rendered when route changes. I don't see the subtlety that exists and isn't exposed.

A hook to render the component on route change and give the route could be easily implemented like:

fn use_route<R: Routable + 'static>() -> &R {
    let route = use_state(|| None);
    use_effect(|| {
        let handle = attach_route_listener(Callback::from(move |new_route| route.set(new_route)));
        || drop(handle)
    })

    &*route
}

(not sure if this snippet is fully correct and compiles but you get the idea of what i'm trying to say)

I didn't add this in the PR because I wanted to avoid dependency on yew-functional for the time being and #1842 would've merged yew-functional in yew so the hook could be added then.

[Diggsey]

Why should it? current_route() means I want the current route at the time of calling and that's exactly what it provides.

I know that, but I think it's easy to misuse, especially as users might expect the component to get re-rendered when the route changes anyway.

It gets detached automatically when the handle is destroyed.

Ah OK. That definitely makes it more usable.

I'm not sure what Mount provides. Can you explain that?

Here's a small example:

// Top-level routes
#[derive(Routable, PartialEq, Clone, Debug)]
pub enum Routes {
    #[at("/")]
    Home,

    // Delegates everything under `/settings` to the `SettingsRoutes` type.
    #[at("/settings")]
    Settings(#[bind(rest)] SettingsRoutes),
}

// Routes within the "Settings" application.
#[derive(Routable, PartialEq, Clone, Debug)]
pub enum SettingsRoutes {
    #[at("/")]
    List,
    #[at("/account")]
    Account,
    #[at("/notifications")]
    Notifications,
}

#[function_component(App)]
fn app() -> Html {
    let router = use_router();

    // This is called when the user navigates *within* the settings application in order to map the
    // `SettingsRoutes` back to a `Routes` type.
    let on_settings_route = router.dispatcher(|sub_route| Some(sub_route.map(Routes::Settings)));

    match &*router.route() {
        Routes::Home => {...},

        // Mount the entire "Settings" application under this route
        Routes::Settings(sub_route) => html! {
            <Mount route=sub_route onroute=on_settings_route>
                <Settings />
            <Mount>
        }
    }
}

#[function_component(Settings)]
fn settings() -> Html {
    let router = use_router();

    match &*router.route() {
        // Here you can see that the "Settings" application doesn't need to understand its place in the application.
        // It can directly create links to `SettingsRoutes`. The parent `Mount` component will take care of the mapping.
        // This `Settings` application could even be mounted at multiple routes within the application.
        SettingsRoutes::List => html! {
            <Link route=SettingsRoute::Account>Account</Link>
            <Link route=SettingsRoute::Notifications>Notifications</Link>
        },
        SettingsRoutes::Account => {...},
        SettingsRoutes::Notifications => {...},
    }
}

[ranile]

I know that, but I think it's easy to misuse, especially as users might expect the component to get re-rendered when the route changes anyway.

I don't think so. It's supposed to be called in view method so when the route is fetched at the time of render. We can ensure that the confusion doesn't happen by documenting it properly.

Mount looks like something that should be kept. I will update my previous comment accordingly.

Also, I'd be happy to work with you on updating my implementation with the features I described so we can have the router provide features without unneeded complexity in the implementation.

[Diggsey]

I don't think so. It's supposed to be called in view method so when the route is fetched at the time of render. We can ensure that the confusion doesn't happen by documenting it properly.

OK - but I think it's important that the "reactive" option is as easy to use as the "non-reactive" option.

Also, I'd be happy to work with you on updating my implementation with the features I described so we can have the router provide features without unneeded complexity in the implementation.

Please go ahead! As mentioned, the Mount functionality precludes the exact API that existed previously, so it would be great if you could outline what you would want the API to look like (whilst still supporting Mount).

Supporting Mount requires that reading/writing the current route goes via a component link. (But we could allow direct access to the "top-level" route as well - with the caveat that it should not be the default or else Mount is not that useful).

That could be easily fixed by adding a thread_local. That solution wouldn't be complex implementation wise and would do the job well.

I'm not sure how that would work - one of the reason I use an agent is because it manages the startup and shutdown of the router automatically.

I'm guessing you would say to reintroduce a Router component and have that startup/shutdown the router, but that means you are forced to use the RenderFn callback to access the route, or else you'd be trying to access it before it was initialized - in my current implementation you can directly access the current route from your application component (or indeed anywhere).

[ranile]

OK - but I think it's important that the "reactive" option is as easy to use as the "non-reactive" option.

I think it is, for struct components. For function components, adding a hook would make it easier.

Please go ahead! As mentioned, the Mount functionality precludes the exact API that existed previously, so it would be great if you could outline what you would want the API to look like (whilst still supporting Mount).

Supporting Mount requires that reading/writing the current route goes via a component link. (But we could allow direct access to the "top-level" route as well - with the caveat that it should not be the default or else Mount is not that useful).

Mount provides nested routing functionality. How do you feel about the API I described in #1853?

I'm not sure how that would work

I was thinking of adding a function in Routable which uses a thread_local generated by the Routable macro.

thread_local!(pub static ROUTE: Option<TheRoutableEnum> = None);
// ...
fn route() -> Option<Self> {
    ROUTE.with(|route| match route {
        Some(route) => route,
        None => Self::recognize()
    })
}

and we update the ROUTE in recognize.

[Diggsey]

Mount provides nested routing functionality. How do you feel about the API I described in #1853?

There are two downsides to that which I wanted to avoid:

1. The subroute has to know where it's going to be mounted, in order to apply the correct mount_at attribute.

2. The parent route can't have its own bound arguments (whether that's URL arguments, query parameters, state, etc.)

With this PR, you can do stuff like:

#[derive(Routable, PartialEq, Clone, Debug)]
pub enum Routes {
    #[at("/")]
    Home,

    #[at("/resources/:id")]
    Resource {
        id: Uuid,
        #[bind(rest)]
        sub_route: ResourceRoutes,
    }
}

Or, let's say you want to still have "application-wide" query parameters, you could use:

#[derive(Routable, PartialEq, Clone, Debug)]
pub enum Routes {
    #[at("/")]
    All {
        #[bind(rest)]
        sub_route: AppRoutes,

        // This query argument is extracted regardless of what route we're visiting
        #[bind(query_arg)]
        token: String,
    }
}

#[derive(Routable, PartialEq, Clone, Debug)]
pub enum AppRoutes {
    #[at("/")]
    Home,
    ...
}

[ranile]

1. The subroute has to know where it's going to be mounted, in order to apply the correct mount_at attribute.

While avoiding that, we end up adding this line (taken from a previous comment of yours):

    // This is called when the user navigates *within* the settings application in order to map the
    // `SettingsRoutes` back to a `Routes` type.
    let on_settings_route = router.dispatcher(|sub_route| Some(sub_route.map(Routes::Settings)));

I'm not sure if that's worth the trade-off.

Also, I'd appreciate if you could provide some implementation details if something like this were to be implemented for the current router

[Diggsey]

While avoiding that, we end up adding this line (taken from a previous comment of yours):

True, but that line makes it considerably more powerful, as you can map the sub-route to the parent however you want (the parent doesn't even necessarily have to store the sub-route, or it could take the entire sub-route and store it as a query parameter or even state).

For the simple case of a new-type wrapper (which is the only kind a mount_at attribute would support) there's plenty of room to introduce a shorthand. For example, the #[derive] macro could provide an Into conversion to go from the sub-route to the parent route automatically, and then the Mount component could use that conversion as a "default" if it exists, as just one possibility.

[ranile]

True. Do you know of any way to implement Mount with the current router?

[Diggsey]

The main issue is that Mount uses contexts to work, and you obviously need a component link to access the context.

There is one other option: we could keep a map of "Route type -> Router" in a static variable. When a Mount component is created, it adds an entry to the map with the appropriate up/down conversion. When the Mount component is destroyed, it removes the entry from the map. The various static router functions would access this map.

The downside to this approach is that it "leaks" outside of the Mount component, and it depends on the sub-route being a unique type. The only realistic example I can think of where that would be a problem would be if you had a recursive route, like:

/resource/:id/children/:child_id1/children/:child_id2

With the current PR you could actually model that with a recursive enum, whereas it wouldn't work with the static map example.

[ranile]

The main issue is that Mount uses contexts to work, and you obviously need a component link to access the context.

That isn't really an issue. We can add a wrapper component that provides us the context without a ComponentLink. I'd like to try implementing Mount for the current router but I don't think I'd have the time to do so in the very near future. Can you give it a shot, in a way that doesn't involve agents or almost rewrites the entire router?

[Diggsey]

That isn't really an issue. We can add a wrapper component that provides us the context without a ComponentLink.

You can have a Router component that acts like a ContextConsumer, but it means you can't access the router from your component anymore (only from the Router callback). The current approach where you can receive an update to your component when the route changes would be more flexible...

I'd like to try implementing Mount for the current router but I don't think I'd have the time to do so in the very near future. Can you give it a shot, in a way that doesn't involve agents or almost rewrites the entire router?

All the changes in this PR were necessary to support the features it adds, it's not like I just changed things for fun 😄. Whilst I think there is room for improvements to the surface API, you would quickly find if you attempted to add the same features to the existing implementation, without making these same changes, that it just doesn't work, or else comes with significant downsides.

Things that could be improved in the surface API:

• I would like to maybe remove the Rc<> wrapper from the return type of the route() method. Either have it return a reference, or clone the route instead.

Aside from the surface API, I could maybe work towards an "Agent-lite" concept - it would work like an agent in that you could "subscribe" to it (to ensure things are cleaned up correctly), but access (ie. getting current route, or pushing a route) to it could work more like a service.

[Diggsey]

@hamza1311 I started migrating stuff to the "Agent-lite" model I described earlier. You can see an example of specifically the HistoryAgent rewrite here:

struct HistoryState {
    last_route: Rc<Route>,
    subscribers: Vec<Callback<Rc<Route>>>,
    cb: Closure<dyn Fn()>,
}

thread_local! {
    static HISTORY_STATE: RefCell<Option<HistoryState>> = RefCell::new(None);
}

/// Service to interface with the history API.
#[derive(Debug)]
pub struct HistoryService;

impl HistoryState {
    fn with<R>(f: impl FnOnce(&mut Option<Self>) -> R) -> R {
        HISTORY_STATE.with(|state| f(&mut *state.borrow_mut()))
    }
    fn determine_current_route() -> Route {
        let window = window();
        let history = window.history().expect("no history");
        let location = window.location();
        let path = location.pathname().expect("no pathname");
        let query = location.search().expect("no pathname");
        let hash = location.hash().expect("no pathname");
        let state = history.state().expect("no state");
        let mut res = Route {
            path,
            query,
            hash,
            state,
        };
        res.unapply_base();
        res
    }

    fn new() -> Self {
        // Install event listeners
        let cb: Closure<dyn Fn()> = Closure::wrap(Box::new(Self::update));

        let et: EventTarget = window().into();
        et.add_event_listener_with_callback("popstate", cb.as_ref().unchecked_ref())
            .expect("add popstate listener");
        et.add_event_listener_with_callback("hashchange", cb.as_ref().unchecked_ref())
            .expect("add hashchange listener");

        Self {
            last_route: Rc::new(Self::determine_current_route()),
            subscribers: Vec::new(),
            cb,
        }
    }

    // We sometimes return a function to run when the state is not borrowed.
    // This is so that callbacks don't panic if they try to access the state.
    fn update_inner(maybe_state: &mut Option<Self>) -> Option<impl FnOnce() + 'static> {
        let state = maybe_state.as_mut()?;
        if state.subscribers.is_empty() {
            *maybe_state = None;
            None
        } else {
            let route = Rc::new(Self::determine_current_route());
            if state.last_route != route {
                state.last_route = route.clone();
                let subscribers = state.subscribers.clone();
                Some(move || {
                    for subscriber in subscribers {
                        subscriber.emit(route.clone());
                    }
                })
            } else {
                None
            }
        }
    }

    fn update() {
        if let Some(f) = Self::with(Self::update_inner) {
            f();
        }
    }

    fn current_route(maybe_state: &mut Option<Self>) -> Rc<Route> {
        maybe_state.get_or_insert_with(Self::new).last_route.clone()
    }

    fn register(maybe_state: &mut Option<Self>, callback: Callback<Rc<Route>>) -> HistoryListener {
        maybe_state
            .get_or_insert_with(Self::new)
            .subscribers
            .push(callback.clone());
        HistoryListener(callback)
    }

    fn unregister(maybe_state: &mut Option<Self>, callback: &Callback<Rc<Route>>) {
        if let Some(state) = maybe_state {
            if let Some(index) = state
                .subscribers
                .iter()
                .position(|subscriber| subscriber == callback)
            {
                state.subscribers.remove(index);
            }
        }
    }
}

impl Drop for HistoryState {
    fn drop(&mut self) {
        let et: EventTarget = window().into();
        et.remove_event_listener_with_callback("popstate", self.cb.as_ref().unchecked_ref())
            .expect("remove popstate listener");
        et.remove_event_listener_with_callback("hashchange", self.cb.as_ref().unchecked_ref())
            .expect("remove hashchange listener");
    }
}

pub struct HistoryListener(Callback<Rc<Route>>);

impl Drop for HistoryListener {
    fn drop(&mut self) {
        HistoryState::with(|state| HistoryState::unregister(state, &self.0));
    }
}

impl HistoryService {
    pub fn dispatch(action: HistoryAction) {
        let history = window().history().expect("no history");
        match action {
            HistoryAction::Push(mut route) => {
                route.apply_base();
                history
                    .push_state_with_url(&route.state, "", Some(&route.url()))
                    .expect("push history");

                // Not triggered automatically by `pushState`.
                HistoryState::update();
            }
            HistoryAction::Replace(mut route) => {
                route.apply_base();
                history
                    .replace_state_with_url(&route.state, "", Some(&route.url()))
                    .expect("replace history");

                // Not triggered automatically by `replaceState`.
                HistoryState::update();
            }
            HistoryAction::Back => history.back().expect("back history"),
            HistoryAction::Forward => history.forward().expect("forward history"),
            HistoryAction::Go(index) => history.go_with_delta(index).expect("go history"),
        }
    }
    pub fn push(route: Route) {
        Self::dispatch(HistoryAction::Push(route));
    }
    pub fn replace(route: Route) {
        Self::dispatch(HistoryAction::Replace(route));
    }
    pub fn forward() {
        Self::dispatch(HistoryAction::Forward);
    }
    pub fn back() {
        Self::dispatch(HistoryAction::Back);
    }
    pub fn go(index: i32) {
        Self::dispatch(HistoryAction::Go(index));
    }
    pub fn current() -> Rc<Route> {
        HistoryState::with(HistoryState::current_route)
    }
    pub fn register(callback: Callback<Rc<Route>>) -> HistoryListener {
        HistoryState::with(|state| HistoryState::register(state, callback))
    }
}

LMK what you think. I plan to make a similar change to convert RouterAgent -> RouterService.

[Diggsey]

Alright, I've made that change, so both RouterAgent and HistoryAgent are now RouterService and HistoryService.

[ranile]

That looks better. From a quick glance, there's 2 things that stand out:

1. structs are used for services. Services are going away altogether from Yew as part of Tracking issue for refactor and cleanup of the codebase #1841 so we should use a different API.
2. You use web_sys directly for attaching event listeners. You should use gloo. The API is much cleaner and it takes care unsubscribing for you.

[Diggsey]

@hamza1311

• I made both of those changes you suggested.
• I reworked the Router type so that you have to explicitly "register" on it to be notified of changes (the constructor only requires a link now). This required some changes to the context API which I'd previously added, so you may want to review that too.
• I added the RouterConsumer component as an alternative to access the router from standard components.

LMK what you think now.

I considered making the functions on Router be standalone (and have the caller pass in the link) but it still seemed worthwhile to have it be a struct as looking up the context is not completely free.

[voidpumpkin]

Looks to me like no one is working on this PR directly, so I will close it. Please notify me if im wrong, i can reopen then.