Skip to content
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

Routable improvements #1461

Merged
merged 7 commits into from Sep 13, 2023
Merged

Routable improvements #1461

merged 7 commits into from Sep 13, 2023

Conversation

tigerros
Copy link
Contributor

@tigerros tigerros commented Sep 13, 2023

  • Fixes Routable::static_routes() always produces an empty result #1451. Should also be a bit faster now.
  • Adds:
    type SiteMapFlattened<'a> = FlatMap<
        Iter<'a, SiteMapSegment>,
        Vec<Vec<SegmentType>>,
        fn(&SiteMapSegment) -> Vec<Vec<SegmentType>>,
    >;
    This is used in the methods below.
  • Adds more methods to Routable. These are:
    • Similar methods to Routable::static_routes():
      • all_routes() -> Vec<Routable>
      • dynamic_routes() -> Vec<Routable>
      • catch_all_routes() -> Vec<Routable>
    • Utility methods:
      • flatten_site_map<'a>() -> SiteMapFlattened<'a> - Returns a flattened version of Self::SITE_MAP.
      • /// Calls a [`Iterator::filter_map`] on [`SiteMapFlattened`].
        fn filter_map_routes<'a, B, F>(f: F) -> FilterMap<SiteMapFlattened<'a>, F>
        where F: FnMut(Vec<SegmentType>) -> Option<B>

@ealmloff
Copy link
Member

This is used in the methods below.

  • Adds more methods to Routable. These are:

    • Similar methods to Routable::static_routes():

      • all_routes() -> Vec<Routable>
      • dynamic_routes() -> Vec<Routable>
      • catch_all_routes() -> Vec<Routable>

I can't think of a good usecase for these methods. The route enum is an instance of the route. It is a specific route, not the template from which routes can be made from (the site map). All of these methods return a instance of the route, not the route definition. For static routes, both the instance of a specific route and the route template are the same because there are no dynamic parameters.

The way the code is structured currently, it looks like the dynamic parameters are filled in with the name of the dynamic parameter. This can cause parsing the route to behave unexpectedly. For example, this code:

#![allow(non_snake_case)]

use dioxus::prelude::*;
use dioxus_router::prelude::*;

fn main() {
    dioxus_web::launch(App);
}

#[derive(Routable, Clone, Debug)]
enum Route {
    #[route("/")]
    Home {},
    #[route("/:id")]
    TakesU64 { id: u64 },
    #[route("/:string")]
    TakesString { string: String },
    #[route("/:..route")]
    TakesSpread {
        route: Vec<String>,
    },
}

fn App(cx: Scope) -> Element {
    render! {
        Router::<Route> {}
    }
}

fn Home(cx: Scope) -> Element {
    render! {
        h1 { "Welcome to the Dioxus Blog!" }
        pre {
            "all_routes:\n{Route::all_routes():?}"
        }
        pre {
            "static_routes:\n{Route::static_routes():?}"
        }
        pre {
            "dynamic_routes:\n{Route::dynamic_routes():?}"
        }
        pre {
            "catch_all_routes:\n{Route::catch_all_routes():?}"
        }
    }
}

fn Blog(cx: Scope) -> Element {
    todo!()
}

#[inline_props]
fn TakesString(cx: Scope, string: String) -> Element {
   todo!()
}

#[inline_props]
fn TakesU64(cx: Scope, id: u64) -> Element {
    todo!()
}

#[inline_props]
fn TakesSpread(cx: Scope, route: Vec<String>) -> Element {
    render! {
        h1 { "Page not found" }
        p { "We are terribly sorry, but the page you requested doesn't exist." }
        pre {
            color: "red",
            "log:\nattemped to navigate to: {route:?}"
        }
    }
}

Displays this site:
Screenshot 2023-09-13 at 9 49 13 AM

  • Utility methods:

    • flatten_site_map<'a>() -> SiteMapFlattened<'a> - Returns a flattened version of Self::SITE_MAP.
    • /// Calls a [`Iterator::filter_map`] on [`SiteMapFlattened`].
      fn filter_map_routes<'a, B, F>(f: F) -> FilterMap<SiteMapFlattened<'a>, F>
      where F: FnMut(Vec<SegmentType>) -> Option<B>

flatten_site_map and SiteMapFlattened is nice! filter_map_routes seems a bit redundant if you can use Route::flatten_site_map().filter_map(f) directly

@ealmloff ealmloff added bug Something isn't working enhancement New feature or request router Related to the router implementation labels Sep 13, 2023
@tigerros
Copy link
Contributor Author

All of these methods return a instance of the route, not the route definition.

I've changed them to instead return the definition. I can't test it because I get an error for derive(Routable):

method `render` has an incompatible type for trait

Note: expected signature `fn(&Route, &'a dioxus_core::scopes::ScopeState, _) -> Option<dioxus_core::nodes::VNode<'a>>`
found signature `fn(&Route, &ScopeState, _) -> Option<VNode<'_>>

I'm surprised it works for you. I have the same exact code in a clean project as you, but still.

@ealmloff
Copy link
Member

All of these methods return a instance of the route, not the route definition.

I've changed them to instead return the definition. I can't test it because I get an error for derive(Routable):

method `render` has an incompatible type for trait

Note: expected signature `fn(&Route, &'a dioxus_core::scopes::ScopeState, _) -> Option<dioxus_core::nodes::VNode<'a>>`
found signature `fn(&Route, &ScopeState, _) -> Option<VNode<'_>>

I'm surprised it works for you. I have the same exact code in a clean project as you, but still.

You need to make the source of the dioxus, dioxus-web, and dioxus-router crates to all match. (in this case have them all point to the same git repo or local workspace)

@tigerros
Copy link
Contributor Author

Thanks. It returns the name of the parameter, not the actual route.

Welcome to the Dioxus Blog!/

all_routes:
["/", "/id", "/string", "/route"]

static_routes:
[Home]

dynamic_routes:
["/id", "/string"]

catch_all_routes:
["/route"]

Not sure if that's useful...

@ealmloff
Copy link
Member

ealmloff commented Sep 13, 2023

Yeah, I don't think it is very useful. We could either remove those functions or return Vec<Vec<RouteSegment>> for all of the non-static-route filters

@tigerros
Copy link
Contributor Author

Removed them. Should be ready to merge when the checks complete.

@ealmloff ealmloff merged commit ae5dca8 into DioxusLabs:master Sep 13, 2023
10 checks passed
@tigerros tigerros deleted the router-fix branch September 13, 2023 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request router Related to the router implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Routable::static_routes() always produces an empty result
2 participants