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.

Sign up for GitHub

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

Jump to bottom

Encode Path Parameters in yew-router #3187

Merged
merged 13 commits into from
Jun 21, 2023
Merged

Encode Path Parameters in yew-router #3187

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
1320c17
Escape for URL in `routable_derive`
Jaffa-Cakes Mar 27, 2023
a1277fa
Fixed generic datatypes bug
Jaffa-Cakes Mar 27, 2023
32f5fe0
FQN
Jaffa-Cakes Mar 28, 2023
b03b361
FQN Again
Jaffa-Cakes Mar 28, 2023
8c02048
Merge branch 'master' into link-path-encode-fix
ranile Jun 1, 2023
07c0ccb
remove encode_for_url from public API
ranile Jun 1, 2023
31ddfa7
fmt
ranile Jun 2, 2023
905cb91
url decode parameters
ranile Jun 13, 2023
3aaea76
add test
ranile Jun 13, 2023
ef727b3
fmt
ranile Jun 13, 2023
29f5439
Merge branch 'master' into link-path-encode-fix
ranile Jun 13, 2023
39dd6b4
remove redundant to_string
ranile Jun 13, 2023
882c500
fmt, please
ranile Jun 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/yew-router-macro/src/routable_derive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))),*)
Copy link
Member

@futursolo futursolo Mar 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Test Source Code

Result

Rendered Result

Copy link
Member

Choose a reason for hiding this comment

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

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/

}
}
Fields::Unnamed(_) => unreachable!(), // already checked
Expand Down
1 change: 1 addition & 0 deletions packages/yew-router/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ route-recognizer = "0.3"
serde = "1"
serde_urlencoded = "0.7.1"
tracing = "0.1.36"
urlencoding = "2"

[dependencies.web-sys]
version = "0.3"
Expand Down
1 change: 1 addition & 0 deletions packages/yew-router/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@futursolo futursolo Apr 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.


pub mod history {
//! A module that provides universal session history and location information.
Expand Down