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

Rewrite router #1791

Merged
merged 58 commits into from
May 17, 2021
Merged

Rewrite router #1791

merged 58 commits into from
May 17, 2021

Conversation

ranile
Copy link
Member

@ranile ranile commented Mar 17, 2021

Description

This PR rewrites the yew-router crate.
I'd appreciate any input about the API of the crate.

This API provided by this PR is a bit different and works as follows:

#[derive(Debug, Clone, Copy, PartialEq, Routable)]
enum Routes {
#[at("/")]
Home,
#[at("/no/:id")]
No { id: u32 },
#[at("/404")]
NotFound,
}
#[derive(Properties, PartialEq, Clone)]
struct NoProps {
id: u32,
}
#[function_component(No)]
fn no(props: &NoProps) -> Html {
let route = props.id.to_string();
html! {
<>
<div id="result-params">{ route }</div>
<div id="result-query">{ service::query().get("foo").unwrap() }</div>
</>
}
}
#[derive(Serialize)]
struct Query {
foo: &'static str,
}
#[function_component(Comp)]
fn component() -> Html {
let switch = Router::render(|routes| {
let onclick = Callback::from(|_| {
service::push_with_query(Routes::No { id: 2 }, Query { foo: "bar" }).unwrap();
});
match routes {
Routes::Home => html! {
<>
<div id="result">{"Home"}</div>
<a onclick=onclick>{"click me"}</a>
</>
},
Routes::No { id } => html! { <No id=id /> },
Routes::NotFound => html! { <div id="result">{"404"}</div> },
}
});
html! {
<Router<Routes> render=switch>
</Router<Routes>>
}
}

A new Routable macro is introduced which generates the implementation for Routable trait.

There will also be another (derive) macro which will be used to allow structs to be created from Params so query parameters are also not stringly typed. I've decided to keep query parameters separate as http-rs/route-recognizer#47 would provide a better and cleaner way to handle those. Right now, URLSearchParams is used for their parsing. A macro should be sufficient for now.

So far, this PR:
Fixes #1245
Fixes #1101
Fixes #1679
Fixes #1102
Fixes #1107
Closes #1615 (the macro is removed)
Closes #1575 (the API is completely changed)
Closes #1096 (the API is completely changed)
Closes #1098 (the crate is removed entirely, route_recognizer is used instead)
Closes #1261 (this implementation doesn't deal with state at all)
Closes #1585 (we no longer do the route parsing ourselves, route_recognizer is used instead. Maybe we could convert the route to something that is accepted by route_recognizer or patch it so it can accept different kinds of values)

Checklist

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

@github-actions
Copy link

github-actions bot commented Mar 21, 2021

Visit the preview URL for this PR (updated for commit d664037):

https://yew-rs--pr1791-router-rewrite-mjf0m3ps.web.app

(expires Mon, 24 May 2021 11:04:28 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@ranile ranile marked this pull request as ready for review March 21, 2021 17:52
@ranile
Copy link
Member Author

ranile commented Mar 21, 2021

This PR should be ready for review now. I have updated the description with the latest details and reasons behind the switch (in addition to what's already described in #1789).

I'd love to hear opinions about how the macro to generate consts and enum/functions for statically typed routes should work.

@ranile
Copy link
Member Author

ranile commented Mar 28, 2021

I found a way to relatively elegantly use enum to define routes without any major compromises. I have updated the PR with the new API and added a macro for it.

At this point, some tests are missing and some fail. Also, the example doesn't compile right now. I'll be marking this PR as draft until those are fixed but this code should be ready to be reviewed.

@ranile ranile marked this pull request as draft March 28, 2021 17:25
@ranile ranile force-pushed the router-rewrite branch 2 times, most recently from 53adbee to 6fcdfdc Compare March 31, 2021 09:15
siku2
siku2 previously approved these changes May 16, 2021
Copy link
Member

@siku2 siku2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like all that's left is to fix the issues from CI.

@mergify mergify bot dismissed siku2’s stale review May 17, 2021 09:00

Pull request has been modified.

@ranile
Copy link
Member Author

ranile commented May 17, 2021

Everything related to this PR should be good in CI now.

@siku2 siku2 added breaking change macro Issues relating to our procedural or declarative macros A-yew-router Area: The yew-router crate labels May 17, 2021
@siku2 siku2 added this to the v0.19 milestone May 17, 2021
@siku2 siku2 merged commit af44076 into yewstack:master May 17, 2021
@siku2
Copy link
Member

siku2 commented May 17, 2021

Thanks a ton, @hamza1311!

One thing that's still kind of lacking from this PR is the documentation. Specifically:

  • how to specify route parameters (describing the syntax and the possibilities, maybe link to the route recognizer)
  • examples for parsing and pushing query parameters

@ranile
Copy link
Member Author

ranile commented May 17, 2021

Thanks a ton, @hamza1311!

One thing that's still kind of lacking from this PR is the documentation. Specifically:

  • how to specify route parameters (describing the syntax and the possibilities, maybe link to the route recognizer)

  • examples for parsing and pushing query parameters

I'll create an issue for that (and also for some other features)

@lausek
Copy link
Contributor

lausek commented Nov 16, 2021

How am I supposed to access BrowserRouter in regular Components?

@voidpumpkin
Copy link
Member

voidpumpkin commented Nov 16, 2021

How am I supposed to access BrowserRouter in regular Components?

@lausek Im going to guess u need history instead BrowserRouter. BrowserRouter is just a context provider you need to wrap your app with.
To get history:
ctx.link().history()

@ranile ranile removed this from the Upcoming release milestone Nov 24, 2021
@voidpumpkin voidpumpkin added A-yew-macro Area: The yew-macro crate A-yew-router-macro Area: The yew-router-macro crate and removed macro Issues relating to our procedural or declarative macros A-yew-macro Area: The yew-macro crate labels Nov 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment