Trailing slashes (#2154) take 2 #2217
Conversation
skirsdeda
force-pushed
the
trailing_slashes
branch
from
ac657bb to
61351ee
Compare
|
There are still differences in path matching corner-cases. In router example '//settings' or '/settings/' or '//settings/' (actual path entered in browser) no longer match settings page but it works fine in previous implementation. |
|
Thanks for updating this! I'm leaving a comment here just to note that I'll need to set aside some time to do a more in depth review, hopefully in the near future. I am going to be doing a pretty large router rewrite for 0.7, and will build this into it in any case. I'll try to get to this some time soon, but in the meantime it is good to be able to point people to this branch as a solution for the particular situation. I'm grateful for your work. |
| /// Overrides any setting applied to [`crate::components::Router`]. | ||
| /// Serves as a default for any inner Routes. | ||
| #[prop(optional)] | ||
| trailing_slash: Option<TrailingSlash>, |
There was a problem hiding this comment.
Why is this being stored as an option? if there is a default value for TrailingSlash why don't we let #[prop(optional)] do the work of setting it to its default value.
There was a problem hiding this comment.
Because None means "inherit trailing_slash setting from parent routes" (see routes::inherit_settings https://github.com/leptos-rs/leptos/pull/2217/files#diff-31f96b333e0c4d9391387d676c81a652b1a975d984ee51b3e2cbd1d4ecf8de8cR352)
| /// Overrides any setting applied to [`crate::components::Router`]. | ||
| /// Serves as a default for any inner Routes. | ||
| #[prop(optional)] | ||
| trailing_slash: Option<TrailingSlash>, |
There was a problem hiding this comment.
Same as for Route.
| /// Overrides any setting applied to [`crate::components::Router`]. | ||
| /// Serves as a default for any inner Routes. | ||
| #[prop(optional)] | ||
| trailing_slash: Option<TrailingSlash>, |
There was a problem hiding this comment.
Same as for Route.
| use std::{cell::RefCell, rc::Rc}; | ||
|
|
||
| #[component] | ||
| fn DefaultApp() -> impl IntoView { |
There was a problem hiding this comment.
maybe add a redundant test for Drop in case the default ever changes?
|
@skirsdeda I have reviewed this and tested it out in some depth, including against the examples that caused broken behavior last time, and it looks to me like this is right. I agree with zoomiti's comments that if the trailing slash props implement If you address that then I think this should be ready to merge. |
|
@gbj |
* retain trailing slashes in paths but leave matching trail-slash-insensitive * fix: Allow trailing slashes to remain in leptos_path. * Better handling for trailing slashes. (leptos-rs#2154) This adds a trailing_slash option to <Router> and <Route>. By default, this option is backward compatible with current Leptos behavior, but users can opt into two new modes for handling trailing slashes. * cargo fmt * Fix redirect routes for wildcard patterns. * Clippy fixies * (Re)Reduce the scope of PossibleBranchContext's internals. * Test real code, not copied code. * Test TrailingSlash redirects. * Fixes and more tests for matching "" && "/". This path is the exception to the rule and *should* be treated as equivalent regardless of its trailing slash. * cargo fmt --------- Co-authored-by: Tadas Dailyda <[email protected]>
skirsdeda
force-pushed
the
trailing_slashes
branch
from
4fea0c6 to
f867c63
Compare
|
Okay, thanks very much for the updates and for all your work on this! Merging now 🎉 |
Fixes regression in #2172. The problem was that it would count 1 path segment in root path ("/") instead of zero. Just moved root path special case to segment extractor function.
Router example tests now pass, also made sure router tests pass (at least with "ssr" feature). Might have missed smth else.