Encode Path Parameters in `yew-router` #3187

Jaffa-Cakes · 2023-03-27T13:58:24Z

Description

This PR fixes #3182, extended information on the bug can be found within the issue by @ColonelThirtyTwo.

The fix makes an amendment to the routable_derive macro in the yew-router and yew-router-macro crates by automatically escaping URL parameters.

I've added urlencoding as a dependency to yew-router to handle the encoding.

Checklist

I have reviewed my own code
I have added tests

I'm unsure if and how I should write a test for this.

Reproduction Repo

I've created a repo that can help you see the issue based on the code @ColonelThirtyTwo included in their issue.

Clone the repo alongside an up-to-date version of the Yew repo. You can then run the basic Yew app and swap between the master branch and this PRs branch to see the fix in action.

github-actions · 2023-03-27T14:02:40Z

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

https://yew-rs-api--pr3187-link-path-encode-fix-eq1fhuni.web.app

_{(expires Wed, 21 Jun 2023 14:11:21 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

github-actions · 2023-03-27T14:05:51Z

Size Comparison

examples	master (KB)	pull request (KB)	diff (KB)	diff (%)
async_clock	103.206	103.206	0	0.000%
boids	176.751	176.751	0	0.000%
communication_child_to_parent	95.775	95.775	0	0.000%
communication_grandchild_with_grandparent	109.828	109.828	0	0.000%
communication_grandparent_to_grandchild	106.326	106.326	0	0.000%
communication_parent_to_child	93.110	93.110	0	0.000%
contexts	111.856	111.856	0	0.000%
counter	89.610	89.610	0	0.000%
counter_functional	90.392	90.392	0	0.000%
dyn_create_destroy_apps	92.759	92.759	0	0.000%
file_upload	103.766	103.766	0	0.000%
function_memory_game	175.182	175.182	0	0.000%
function_router	346.308	348.712	+2.404	+0.694%
function_todomvc	164.087	164.087	0	0.000%
futures	227.683	227.683	0	0.000%
game_of_life	112.262	112.262	0	0.000%
immutable	188.909	188.909	0	0.000%
inner_html	86.386	86.386	0	0.000%
js_callback	113.853	113.853	0	0.000%
keyed_list	202.078	202.078	0	0.000%
mount_point	89.578	89.578	0	0.000%
nested_list	114.460	114.460	0	0.000%
node_refs	96.642	96.642	0	0.000%
password_strength	1589.263	1589.263	0	0.000%
portals	99.099	99.099	0	0.000%
router	312.157	314.598	+2.440	+0.782%
simple_ssr	145.125	145.125	0	0.000%
ssr_router	383.677	386.125	+2.448	+0.638%
suspense	111.119	111.119	0	0.000%
timer	92.288	92.288	0	0.000%
timer_functional	100.930	100.930	0	0.000%
todomvc	143.966	143.966	0	0.000%
two_apps	90.256	90.256	0	0.000%
web_worker_fib	154.971	154.971	0	0.000%
webgl	88.895	88.895	0	0.000%

✅ None of the examples has changed their size significantly.

futursolo · 2023-03-27T14:04:47Z

packages/yew-router-macro/src/routable_derive.rs

@@ -176,7 +176,7 @@ impl Routable {
                    }

                    quote! {
-                        Self::#ident { #(#fields),* } => ::std::format!(#right, #(#fields = #fields),*)
+                        Self::#ident { #(#fields),* } => ::std::format!(#right, #(#fields = yew_router::encode_for_url(&format!("{}", #fields))),*)


I have not got time to test this change, but I think this should not be applied on the remainder path matcher(*) which would otherwise escape the remainder path.

I've just done a quick test and it seems to work correctly in its current state when the link is generated and clicked.

Is this what you were concerned about failing?

Code

Result

In route recognizer, you can construct a route like /external/*remainder (referred to as "named wildcards" in their documentation) which it will match both /external/a and /external/a/b/c/d and collects a and a/b/c/d into the route parameter respectively.
If we apply escaping to this, then we will not be able to construct routes like /external/a/b/c/d with Link.

See: https://docs.rs/route-recognizer/latest/route_recognizer/

github-actions · 2023-03-27T14:07:08Z

Benchmark - SSR

Yew Master

Benchmark	Round	Min (ms)	Max (ms)	Mean (ms)	Standard Deviation
Baseline	10	319.554	320.242	319.761	0.234
Hello World	10	617.271	618.849	617.951	0.555
Function Router	10	2054.100	2070.479	2061.075	5.835
Concurrent Task	10	1007.048	1008.667	1007.879	0.501
Many Providers	10	1487.112	1520.135	1497.779	11.509

Pull Request

Benchmark	Round	Min (ms)	Max (ms)	Mean (ms)	Standard Deviation
Baseline	10	357.311	357.932	357.580	0.188
Hello World	10	635.133	637.065	636.090	0.701
Function Router	10	2033.944	2053.809	2041.395	6.001
Concurrent Task	10	1007.557	1008.402	1008.005	0.304
Many Providers	10	1455.666	1479.565	1467.195	7.528

Jaffa-Cakes · 2023-03-28T05:39:54Z

It seems as though the unit tests on Rust 1.60 aren't successful, but the unit tests on the Rust Stable and Nightly are alright.

Will this be an issue @futursolo?

futursolo · 2023-03-28T05:47:26Z

This failure is due to macro hygiene.
Macros are only tested against MSRV.

To fix the failures, you need to use fully qualified paths for all invocations from the definition site.

Jaffa-Cakes · 2023-03-28T06:59:56Z

@futursolo

To fix the failures, you need to use fully qualified paths for all invocations from the definition site.

That's fixed now.

futursolo · 2023-03-28T10:23:08Z

packages/yew-router/src/lib.rs

@@ -80,6 +80,7 @@ pub mod utils;
 pub use routable::{AnyRoute, Routable};
 pub use router::{BrowserRouter, HashRouter, Router};
 pub use switch::Switch;
+pub use urlencoding::encode as encode_for_url;


I think this should be exposed via "macro_herlpers.rs".

In addition, it might worth to test js_sys::encode_uri_component and prefer it under wasm targets and see if using the function provided by js_sys can avoid the size increase.

I agree that this should be exposed via macro_helpers and certainly not be a part of a public API. @Jaffa-Cakes please move it over to that file.

I don't think js_sys::encode_uri_component is worth it here though. The binary size increase is minimal and the JS round trip: (UTF8 (rust memory) -> UTF16 (JS memory) -> UTF8 (rust memory)) may be too costly

I think the primary performance hit when routes are switched comes from rendering components of the new page after the URL switch. I feel it might be worth to avoid the size increase here as we are looking at 5% increase of example sizes for the next release based on upcoming pull requests (e.g.: #3169, #3050) and the other size increases are not likely avoidable.

But I don't have a high preference over either choice, so I am fine with proceeding with the Rust dependency as well.

futursolo

Thank you for fixing this issue!

This pull request in general looks good. However, I think if it encodes the parameter for the route, then it should also decode the route parameters automatically after the route is parsed.

So that, for the following route:

#[derive(Routable, Debug, Clone, PartialEq, Eq)]
pub enum Route {
    #[at("/articles/:name")]
    Articles { name: String }
}

When name is: "a/b",
<Link<Route> to={Route::Articles { name: "a/b".to_string() }} /> takes us to /articles/a%2Fb and the render function of <Switch<Route> render={...} /> will get Route::Articles { name: "a/b".to_string() } as the function argument.

In addition, could you please also add a test case to this pull request?

ranile · 2023-04-01T22:07:54Z

packages/yew-router/src/lib.rs

@@ -80,6 +80,7 @@ pub mod utils;
 pub use routable::{AnyRoute, Routable};
 pub use router::{BrowserRouter, HashRouter, Router};
 pub use switch::Switch;
+pub use urlencoding::encode as encode_for_url;


I agree that this should be exposed via macro_helpers and certainly not be a part of a public API. @Jaffa-Cakes please move it over to that file.

I don't think js_sys::encode_uri_component is worth it here though. The binary size increase is minimal and the JS round trip: (UTF8 (rust memory) -> UTF16 (JS memory) -> UTF8 (rust memory)) may be too costly

# Conflicts: # packages/yew-router/Cargo.toml

ranile · 2023-06-01T12:44:02Z

I was going to update the PR but it looks like @Jaffa-Cakes didn't give the permissions to do so. @Jaffa-Cakes, if you don't want to work on it, would you mind if I created a new PR with the changes?

Jaffa-Cakes · 2023-06-01T12:49:20Z

@hamza1311 I have enabled maintainer edits now for you if you like, or you could make a new PR, your preference.

ranile · 2023-06-02T18:12:26Z

CI failure is tracing bug: tokio-rs/tracing#2503

I updated the PR so my review now stands dismissed

futursolo

I think this comment has yet to be addressed. You need to call urlencoding::decode to convert a%2Fb back to a/b.

Before tracing gets to fix the lint, maybe it's worth to duplicate the render_stream implementation with #[rustversion::since(1.70)] and #[rustversion::before(1.70)] and only apply the lint on the implementation that is enabled after 1.70. So CI can run clippy properly.

Jaffa-Cakes added 2 commits March 27, 2023 23:40

Escape for URL in routable_derive

1320c17

Fixed generic datatypes bug

a1277fa

futursolo reviewed Mar 27, 2023

View reviewed changes

Jaffa-Cakes mentioned this pull request Mar 28, 2023

yew-router does not encode path parameters #3182

Closed

3 tasks

Jaffa-Cakes added 2 commits March 28, 2023 17:43

FQN

32f5fe0

FQN Again

b03b361

futursolo reviewed Mar 28, 2023

View reviewed changes

futursolo added breaking change bug A-yew-router Area: The yew-router crate A-yew-router-macro Area: The yew-router-macro crate labels Mar 28, 2023

futursolo reviewed Mar 28, 2023

View reviewed changes

ranile previously requested changes Apr 1, 2023

View reviewed changes

futursolo added the S-waiting-on-author Status: awaiting action from the author of the issue/PR label Apr 4, 2023

ranile mentioned this pull request Jun 1, 2023

Reduce bundle size formbird/json-patcher#9

Merged

ranile added 2 commits June 1, 2023 17:27

Merge branch 'master' into link-path-encode-fix

8c02048

# Conflicts: # packages/yew-router/Cargo.toml

remove encode_for_url from public API

07c0ccb

github-actions bot previously approved these changes Jun 1, 2023

View reviewed changes

fmt

31ddfa7

ranile dismissed github-actions[bot]’s stale review via 31ddfa7 June 2, 2023 17:56

github-actions bot previously approved these changes Jun 2, 2023

View reviewed changes

ranile enabled auto-merge (squash) June 2, 2023 18:12

ranile requested a review from futursolo June 2, 2023 18:12

futursolo reviewed Jun 3, 2023

View reviewed changes

ranile added 3 commits June 13, 2023 21:35

url decode parameters

905cb91

add test

3aaea76

fmt

ef727b3

ranile dismissed github-actions[bot]’s stale review via ef727b3 June 13, 2023 16:36

Merge branch 'master' into link-path-encode-fix

29f5439

github-actions bot previously approved these changes Jun 13, 2023

View reviewed changes

remove redundant to_string

39dd6b4

ranile dismissed github-actions[bot]’s stale review via 39dd6b4 June 13, 2023 16:42

github-actions bot previously approved these changes Jun 13, 2023

View reviewed changes

fmt, please

882c500

ranile dismissed github-actions[bot]’s stale review via 882c500 June 14, 2023 14:09

ranile removed the S-waiting-on-author Status: awaiting action from the author of the issue/PR label Jun 14, 2023

ranile requested a review from futursolo June 14, 2023 14:09

github-actions bot approved these changes Jun 14, 2023

View reviewed changes

futursolo approved these changes Jun 21, 2023

View reviewed changes

ranile merged commit 27f537a into yewstack:master Jun 21, 2023

Anshorei mentioned this pull request Nov 3, 2023

Router urlencoding should not encode '/' in *path #3514

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encode Path Parameters in `yew-router` #3187

Encode Path Parameters in `yew-router` #3187

Jaffa-Cakes commented Mar 27, 2023 •

edited

Loading

github-actions bot commented Mar 27, 2023 •

edited

Loading

github-actions bot commented Mar 27, 2023 •

edited

Loading

futursolo Mar 27, 2023 •

edited

Loading

Jaffa-Cakes Mar 28, 2023

futursolo Mar 28, 2023

github-actions bot commented Mar 27, 2023 •

edited

Loading

Jaffa-Cakes commented Mar 28, 2023

futursolo commented Mar 28, 2023 •

edited

Loading

Jaffa-Cakes commented Mar 28, 2023

futursolo Mar 28, 2023

ranile Apr 1, 2023

futursolo Apr 2, 2023 •

edited

Loading

futursolo left a comment •

edited

Loading

ranile Apr 1, 2023

ranile commented Jun 1, 2023

Jaffa-Cakes commented Jun 1, 2023

ranile commented Jun 2, 2023

futursolo left a comment

Encode Path Parameters in yew-router #3187

Encode Path Parameters in yew-router #3187

Conversation

Jaffa-Cakes commented Mar 27, 2023 • edited Loading

Description

Checklist

Reproduction Repo

github-actions bot commented Mar 27, 2023 • edited Loading

github-actions bot commented Mar 27, 2023 • edited Loading

Size Comparison

futursolo Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Jaffa-Cakes Mar 28, 2023

Choose a reason for hiding this comment

Code

Result

futursolo Mar 28, 2023

Choose a reason for hiding this comment

github-actions bot commented Mar 27, 2023 • edited Loading

Benchmark - SSR

Yew Master

Pull Request

Jaffa-Cakes commented Mar 28, 2023

futursolo commented Mar 28, 2023 • edited Loading

Jaffa-Cakes commented Mar 28, 2023

futursolo Mar 28, 2023

Choose a reason for hiding this comment

ranile Apr 1, 2023

Choose a reason for hiding this comment

futursolo Apr 2, 2023 • edited Loading

Choose a reason for hiding this comment

futursolo left a comment • edited Loading

Choose a reason for hiding this comment

ranile Apr 1, 2023

Choose a reason for hiding this comment

ranile commented Jun 1, 2023

Jaffa-Cakes commented Jun 1, 2023

ranile commented Jun 2, 2023

futursolo left a comment

Choose a reason for hiding this comment

Encode Path Parameters in `yew-router` #3187

Encode Path Parameters in `yew-router` #3187

Jaffa-Cakes commented Mar 27, 2023 •

edited

Loading

github-actions bot commented Mar 27, 2023 •

edited

Loading

github-actions bot commented Mar 27, 2023 •

edited

Loading

futursolo Mar 27, 2023 •

edited

Loading

github-actions bot commented Mar 27, 2023 •

edited

Loading

futursolo commented Mar 28, 2023 •

edited

Loading

futursolo Apr 2, 2023 •

edited

Loading

futursolo left a comment •

edited

Loading