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 does not match #[not_found] if it fails to parse into the target type #2106

Closed
1 of 3 tasks
futursolo opened this issue Oct 9, 2021 · 3 comments · Fixed by #2107
Closed
1 of 3 tasks

Router does not match #[not_found] if it fails to parse into the target type #2106

futursolo opened this issue Oct 9, 2021 · 3 comments · Fixed by #2107
Labels
A-yew-router Area: The yew-router crate bug

Comments

@futursolo
Copy link
Member

futursolo commented Oct 9, 2021

Problem

#[derive(Routable, Debug, Clone, PartialEq)]
pub(crate) enum AppRoute {
    #[at("/")]
    Home,
    #[at("/:id")]
    Article { id: u64 },
    #[at("/404")]
    #[not_found]
    NotFound,
}

println!("{:?}", AppRoute::recognize("/not/matched/route"));
println!("{:?}", AppRoute::recognize("/not-matched-route"));

And the output is:

Some(NotFound)
None

When using this route with Router, if the path matches /anything-but-u64, the router will simply stop rendering with no route matched.

Expected behavior

The output of the snippet above should be:

Some(NotFound)
Some(NotFound)

Screenshots
If applicable, add screenshots to help explain your problem.

Environment:

  • Yew version: master
  • Rust version: 1.55.0
  • Target, if relevant: wasm32-unknown-unknown
  • Build tool, if relevant: not relevant
  • OS, if relevant: not relevant
  • Browser and version, if relevant: not relevant

Questionnaire

  • I'm interested in fixing this myself but don't know where to start
  • I would like to fix and I have a solution
  • I don't have time to fix this right now, but maybe later
@futursolo futursolo added the bug label Oct 9, 2021
@mc1098 mc1098 added the A-yew-router Area: The yew-router crate label Oct 9, 2021
@ranile
Copy link
Member

ranile commented Oct 10, 2021

This behavior is intentional. I kept it that way because more often than not, you would want to display a 404 message related to path segment missing. For example, you might want to display "route article not found". If we were to redirect to /404 in such case, that data would be lost. I think this is something that needs to be discussed before being changed.

@futursolo
Copy link
Member Author

For example, you might want to display "route article not found".

I don't think the current method has a way to recover from the "no route matched" error.
(This error message is shown in console as a warning and renders an empty page.)

If we were to redirect to /404 in such case

#[not_found] currently does not redirect, AppRoute::NotFound variant is returned to render_fn so an 404 page can be render in current path.
which means the path is obtainable via window().location().pathname().

@mc1098
Copy link
Contributor

mc1098 commented Oct 13, 2021

Expected behavior
The output of the snippet above should be:
Some(NotFound)
Some(NotFound)

This is what I'd expect too - #2107 would limit None to mean no router variant matches and there is no defined #[not_found] variant to fall back on.

For example, you might want to display "route article not found"

I think it could be useful to add a debug console log for when from_path fails to parse?
In terms of not finding the article, I'd assume you'd only do this after you got the correctly parsed 'id' and then found that no article exists for that id.

Also when we support nested routers then you could do this easily if you wanted to have a topic specific not found behaviour - though you'd probably have something like this:

#[derive(Routable, Debug, Clone, PartialEq)]
pub(crate) enum AppRoute {
    #[at("/")]
    Home,
    #[at("/article")]
    Article,
    #[at("/404")]
    #[not_found]
    NotFound,
}

// somewhere else
#[derive(Routable, Debug, Clone, PartialEq)]
pub(crate) enum ArticleRoute {
    #[at("/")]
    Articles,
    #[at("/:id")]
    Article { id: u64 },
    #[at("/404")]
    #[not_found]
    NotFound,
}

This is just to give an idea - I know that nested routers are still in flux API wise :)

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 bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants