Repository navigation
fix(fullstack): handle relative base paths in client requests - #5908
Arthur031221 wants to merge 2 commits into
Conversation
Co-authored-by: Noe Terrier <noeterrier@proton.me>
LucaCappelletti94
left a comment
LucaCappelletti94
left a comment
There was a problem hiding this comment.
Not a maintainer, just a fellow contributor. The PR overall looks good, I have just a few nits.
If you happened to have time, I would also appreciate if you could review one of my PRs so as to reduce a bit the workload of the maintainers.
| @@ -33,14 +33,18 @@ impl ClientRequest { | |||
| // Shrink monomorphization bloat by moving this to its own function | |||
| fn fetch_inner(method: http::Method, path: String, query: String) -> ClientRequest { | |||
| // On wasm, this doesn't matter since we always use relative URLs when making requests anyways | |||
There was a problem hiding this comment.
I suggest you rewrite this comment, since it says the server URL does not matter on wasm while the branch below exists exactly because the browser request keeps its path.
| // On wasm, this doesn't matter since we always use relative URLs when making requests anyways | |
| // Browsers send only the path and query, so a relative server URL gets a placeholder origin. |
| set_server_url(server_url); | ||
| let request = ClientRequest::new(Method::GET, "/api/echo".into(), &()); | ||
| assert_eq!(request.url().as_str(), expected_url); | ||
| assert_eq!(request.method(), Method::GET); |
There was a problem hiding this comment.
You should drop this assertion. ClientRequest::new stores the method unchanged, so it cannot fail for any change to base path handling.
| assert_eq!(request.method(), Method::GET); |
| request.url().as_str(), | ||
| format!("{expected_url}?search=a%26b") | ||
| ); | ||
| assert_eq!(request.method(), Method::POST); |
There was a problem hiding this comment.
You should drop this assertion for the same reason as line 33.
| assert_eq!(request.method(), Method::POST); |
Co-authored-by: Luca Cappelletti <7738570+LucaCappelletti94@users.noreply.github.com>
|
Applied the comment change and removed the two method assertions. The fullstack tests pass with and without |
This branch has not been deployed
2 participants
Web fullstack apps with a nonempty
base_pathcan panic when building server function requests because the relative server URL is parsed as an absolute URL.This uses the existing placeholder origin for slash-prefixed base paths. Browser transport uses only the resulting path and query, keeping the configured prefix. This continues #5605 by @NoeTerrier. Fixes #5032.
Added constructor regression coverage, which passes with
cargo test --locked -p dioxus-fullstack.Assisted by Claude/Codex.