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

Router Switch is order dependant #1679

Closed
theduke opened this issue Jan 7, 2021 · 7 comments · Fixed by #1791
Closed

Router Switch is order dependant #1679

theduke opened this issue Jan 7, 2021 · 7 comments · Fixed by #1791
Labels
A-yew-router Area: The yew-router crate ergonomics
Projects

Comments

@theduke
Copy link

theduke commented Jan 7, 2021

Problem

With the below router definition, the router will always match the Home route and ignore the other definitions.

Maybe "/" has some special match-all semantics?

Making Home the last variant will make things work properly.

This is highly confusing behaviour, I spent quite a bit of time tracking this down.

#[derive(yew_router::Switch, Debug, Clone)]
pub enum AppRoute {
   #[to = "/"]
    Home,
    #[to = "/feed/{category}"]
    Feed(String),
    #[to = "/post/{id}"]
    Post(u64),
}

Environment:

  • Yew version: 0.17.4
  • yew-router: 0.14.0
@theduke theduke added the bug label Jan 7, 2021
@siku2 siku2 added the A-yew-router Area: The yew-router crate label Jan 7, 2021
@hamza1311
Copy link
Member

This behavior is documented but I do think it should be made order independent if possible.

@theduke
Copy link
Author

theduke commented Jan 7, 2021

It's not documented though that "/" is essentially a "/*" wildcard.

Would also be good to add the warning from your link to the crate docs. (I never checked the web docs, just docs.rs).

@siku2
Copy link
Member

siku2 commented Jan 7, 2021

This is working as intended, even if is confusing and not appropriately documented.
This is something that should be addressed in router v2.

Off the top of my head I believe there are cases where we need to rely on the order to resolve parsing ambiguities so I don't think we can get rid of it entirely without losing flexibility or redesigning the system. That being said, cases where a route is definitely inaccessible should result in a compilation error. This should hopefully catch most cases where this happens by accident.

@siku2 siku2 added ergonomics and removed bug labels Jan 7, 2021
@siku2 siku2 added this to To do in Router v2 via automation Jan 7, 2021
@siku2
Copy link
Member

siku2 commented Jan 7, 2021

It's not documented though that "/" is essentially a "/*" wildcard.

Would also be good to add the warning from your link to the crate docs. (I never checked the web docs, just docs.rs).

This is something that bothers me as well. It isn't directly related to the ordering but I think it should be addressed as well, because the implicit wildcard is quite confusing.

@lukechu10
Copy link
Contributor

lukechu10 commented Feb 25, 2021

We could potentially inspire from Rocket (routing and dynamic paths). The grammar for Rocket routes is here.

Even though I am not familiar with Rocket, it seems like routing is order independent. For ambiguous routes, there is a well defined "ranking" or manually set with a "rank" attribute on the route attribute.

The lexer generator, logos might also be a source of inspiration with "priorities". In the case of logos, when two routes are ambiguous and have the same priority, a compiler error is emitted. This sounds a bit complicated to implement (there is probably some elegant algorithm out there that I am not aware of) but it's probably necessary to make routing order independent. This issue might have some hints about how to do it: maciejhirsz/logos#129

Lastly, it would be really cool if we could also match regex in routes. Again, logos could be a source of inspiration for this.

Edit: Heck, we could just generate a Logos lexer with the proc_macro and leave all the complexity to Logos. Don't know if this is a good idea or not.

Edit 2: On second thought, all those / would be a nightmare to escape in the regexes so maybe this isn't such a good idea.

Edit 3: Logos does not provide a way for extracting values from sub patterns so it will probably not be practical anyways.

@lukechu10
Copy link
Contributor

lukechu10 commented Feb 25, 2021

I think I know how to do this: we create a DFA (deterministic finite state automata) for each route. Then we combine the DFAs together into one finite state automata for all the routes. If an end node can match two different routes and these two different routes have the same priority, throw an error and abort.

Edit: It seems like the current implementation ships nom in the WASM bundle. Would it be possible to just use UrlSearchParams and some simple regex to extract all the information we need about the path? For the matching, we could have the derive macro emit directly a state machine which could match the url segments.

@lukechu10
Copy link
Contributor

lukechu10 commented Feb 26, 2021

Interestingly enough, it seems like [React router] is order dependent. The main difference I can see with the current yew router is that it's not end recursive by default, meaning / will only match the root page.

I think this is a easy way out and is not the best solution. I am trying to implement a URL matcher based on finite state automata where the DFAs of different routes are merged together to remove any ordering restriction. It's in a separate repo for now but I'll make it public once I get a working demo.

Edit: As a bonus with this method, we can compute the finite state automata completely at compile time in the proc macro and have minimal code shipped to the browser.

@hamza1311 hamza1311 mentioned this issue Mar 21, 2021
3 tasks
Router v2 automation moved this from To do to Done May 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew-router Area: The yew-router crate ergonomics
Projects
No open projects
Router v2
  
Done
Development

Successfully merging a pull request may close this issue.

4 participants