Better handling for trailing slashes. (#2154) #2172

NfNitLoop · 2024-01-08T01:05:33Z

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.

…sitive

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.

NfNitLoop · 2024-01-08T01:06:02Z

Things still WIP:

A few TODOs
Figure out a way to test redirects in unit tests. (They're working for me in Actix locally.)
Are there any tests I'm forgetting to run? cargo +nightly test doesn't work for me on main. So I've been running it in the leptos/, router/ and integrations/actix/ subdirectories.
Confirm: Is this going in the right direction? Should I spend more time on this?

NfNitLoop · 2024-01-08T01:43:53Z

Link to #2154 (the related feature request), please, GitHub! 😊

router/src/extract_routes.rs

router/tests/extract_routes.rs

rakshith-ravi · 2024-01-08T02:04:36Z

Quick question - is this the same as #2163?

NfNitLoop · 2024-01-08T02:10:36Z

@rakshith-ravi #2163 isn't by me, it's by @skirsdeda. But we were discussing it as an incremental approach to this larger feature implementation in the feature request, so I provided some feedback over there and based these changes atop that work.

IMO we probably shouldn't merge #2163 in its current state. Without 10915f9 the front- and back-end routing are slightly different.

rakshith-ravi · 2024-01-08T02:14:45Z

Yeah I got confused with the issue numbers for a second there. Like I said, super sleep deprived rn 😅

so I provided some feedback over there and based these changes atop that work.

Ahh, got it. Greg will have a better idea about this, but just my two cents - how about y'all put a PR on each other's branches and then have the complete slash handling as a single PR to main? That way, it'll be a feature complete PR.

On the flip side, it would mean a larger PR, so reviewing it might be difficult. Idk. Just putting both sides of the argument

NfNitLoop · 2024-01-08T02:34:06Z

how about y'all put a PR on each other's branches and then have the complete slash handling as a single PR to main?

This is the full PR. Minus a couple TODOs and any suggestions I get from maintainers. 😊

router/tests/extract_routes.rs

benwis · 2024-01-08T19:41:26Z

how about y'all put a PR on each other's branches and then have the complete slash handling as a single PR to main?

This is the full PR. Minus a couple TODOs and any suggestions I get from maintainers. 😊

This looks pretty solid to me, I've started the checks. I think we should add some mention to the interplay between the Axum and Actix routers, and the Leptos one. Does Axum behave the same way?

benwis · 2024-01-08T20:30:40Z

Probably also want to run cargo clippy against your repo

NfNitLoop · 2024-01-09T07:46:38Z

Blocker: In testing, I found that these changes break route matching for "" (leptos) and "/" (web servers). I'm capturing that in some tests and will devise a fix but probably not until tomorrow night.

NfNitLoop · 2024-01-09T08:40:11Z

Are all of these test failures in CI valid failures that I need to look into? Or are some just known to be flakey?

benwis · 2024-01-09T17:41:47Z

Are all of these test failures in CI valid failures that I need to look into? Or are some just known to be flakey?

@gbj is probably more familiar than I, but some of the examples test suites are failing now. Is this common?

gbj · 2024-01-09T18:01:15Z

I'll have the CI rerun the failed tasks just in case. I would need to check out the branch and test out the examples myself to give a definitive answer, but I'm seeing failing e2e tests on several examples that do involve routing, so it's worth checking whether those examples actually work with the changes.

NfNitLoop · 2024-01-10T17:53:58Z

OK, thanks. I hope to have more time to look into these tonight. Hopefully it's just the known issue with "/" and "" routes breaking that I mentioned above.

maybe TMI but just to share:

I've already looked into that a bit and found it was because of the changes to path normalization. The problem is that path normalization works the same everywhere … except root paths. For both Actix and Axum, (and HTTP/HTML, AFAICT) root paths of "/" and "" are equivalent. So the error comes when Leptos specifies a root path of "", but the web server gives us a path of "/", which Leptos doesn't find a match for (in this branch, given its more precise path matching.) I've got a couple different fixes I'm testing out.

This path is the exception to the rule and *should* be treated as equivalent regardless of its trailing slash.

NfNitLoop · 2024-01-11T05:56:50Z

OK, I've done local testing with Actix and Axum and have fixed my blocking issue. I bet that was the breaking issue for those integration tests, given they couldn't even render the first page of the site.🤞

I started down the path of trying to run the tests for myself locally but it looks like they require manual installation of some test tools and it's getting late here. If one of y'all can kick these off tonight/tomorrow I'll come back and check results here. Thank you! 😊

benwis · 2024-01-11T19:45:55Z

OK, I've done local testing with Actix and Axum and have fixed my blocking issue. I bet that was the breaking issue for those integration tests, given they couldn't even render the first page of the site.🤞

I started down the path of trying to run the tests for myself locally but it looks like they require manual installation of some test tools and it's getting late here. If one of y'all can kick these off tonight/tomorrow I'll come back and check results here. Thank you! 😊

Looks promising, although it seems to have failed a cargo fmt check

NfNitLoop · 2024-01-11T21:33:55Z

it seems to have failed a cargo fmt check

Weird, I would've sworn I ran cargo fmt before pushing my last commit lsat night. 🤷

Thankfully that's an easy one to fix! I assume/hope we'll squash merge this messy history away? 😅

NfNitLoop · 2024-01-11T22:24:23Z

And merged? Just like that! Thank you so much! I'll definitely be using this in my own project(s)! 😊

benwis · 2024-01-11T22:27:15Z

Yep, the tests passed, it’s a useful feature, and the code’s solid. Thanks for contributing!

…

On Thu, Jan 11, 2024, at 2:24 PM, Cody Casterline wrote: And merged? Just like that! Thank you so much! I'll definitely be using this in my own project(s)! 😊 — Reply to this email directly, view it on GitHub <#2172 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVBTCO5DYIXCYKNOBODGJLYOBRCHAVCNFSM6AAAAABBQXEW3SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBYGA3DKOJTGQ>. You are receiving this because you modified the open/close state.Message ID: ***@***.***>

gbj · 2024-01-19T15:25:40Z

@NfNitLoop @benwis Sorry to say this PR broke the router in a way that unfortunately was not caught by the existing CI setup.

Two examples:

This valid approach for defining a param, and a fallback if the param was not provided, now panics at runtime when Axum tries to create duplicate routes

<Routes>
  <Route path="bar" view=|| view! { <Outlet/> }>
    <Route path="" view=|| view! { <p>"Empty"</p>}/>
    <Route path=":id" view=|| view! { <p>"ID"</p>}/>
  </Route>
</Routes>

Similarly, it breaks matching when there is a param at the root level, as in the router example in the repo or in Incorrect route gets rendered #2201

Given the breakages in existing code I'm going to have to revert this and yank the last two releases, since I unfortunately didn't catch it before then. If you can find a way to implement this that does not cause these issues, I will be happy to review a new PR.

This reverts commit 1eaf886.

* 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 and others added 3 commits January 5, 2024 23:57

retain trailing slashes in paths but leave matching trail-slash-insen…

98baa38

…sitive

fix: Allow trailing slashes to remain in leptos_path.

10915f9

Better handling for trailing slashes. (leptos-rs#2154)

011cea7

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.

Merge remote-tracking branch 'leptos/main' into trailing_slashes

fac2a3c

NfNitLoop changed the title ~~Better handling for trailing slashes. (#2154)Trailing slashes~~ Better handling for trailing slashes. (#2154) Jan 8, 2024

cargo fmt

9aef6f1

NfNitLoop commented Jan 8, 2024

View reviewed changes

router/src/extract_routes.rs Outdated Show resolved Hide resolved

NfNitLoop commented Jan 8, 2024

View reviewed changes

router/tests/extract_routes.rs Outdated Show resolved Hide resolved

Fix redirect routes for wildcard patterns.

777c425

NfNitLoop commented Jan 8, 2024

View reviewed changes

router/tests/extract_routes.rs Outdated Show resolved Hide resolved

skirsdeda mentioned this pull request Jan 8, 2024

router: Retain trailing slashes in paths #2163

Closed

Cody Casterline added 4 commits January 8, 2024 16:54

Clippy fixies

db6cb04

(Re)Reduce the scope of PossibleBranchContext's internals.

334cfd1

Test real code, not copied code.

c5568f3

Test TrailingSlash redirects.

4672d6e

Fixes and more tests for matching "" && "/".

9c379f0

This path is the exception to the rule and *should* be treated as equivalent regardless of its trailing slash.

cargo fmt

a329724

benwis merged commit 1eaf886 into leptos-rs:main Jan 11, 2024
10 checks passed

benwis mentioned this pull request Jan 16, 2024

leptos_router: Feature: Support for trailing slashes. #2154

Closed

gbj mentioned this pull request Jan 19, 2024

Incorrect route gets rendered #2201

Closed

gbj added a commit that referenced this pull request Jan 19, 2024

Revert "Better handling for trailing slashes. (#2154) (#2172)"

ae1de88

This reverts commit 1eaf886.

gbj mentioned this pull request Jan 19, 2024

fix: routing regressions caused by trailing_slash support #2203

Merged

skirsdeda mentioned this pull request Jan 21, 2024

Trailing slashes (#2154) take 2 #2217

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better handling for trailing slashes. (#2154) #2172

Better handling for trailing slashes. (#2154) #2172

NfNitLoop commented Jan 8, 2024

NfNitLoop commented Jan 8, 2024 •

edited

Loading

NfNitLoop commented Jan 8, 2024 •

edited

Loading

rakshith-ravi commented Jan 8, 2024 •

edited

Loading

NfNitLoop commented Jan 8, 2024

rakshith-ravi commented Jan 8, 2024

NfNitLoop commented Jan 8, 2024

benwis commented Jan 8, 2024 •

edited

Loading

benwis commented Jan 8, 2024

NfNitLoop commented Jan 9, 2024 •

edited

Loading

NfNitLoop commented Jan 9, 2024

benwis commented Jan 9, 2024

gbj commented Jan 9, 2024

NfNitLoop commented Jan 10, 2024 •

edited

Loading

NfNitLoop commented Jan 11, 2024

benwis commented Jan 11, 2024

NfNitLoop commented Jan 11, 2024

NfNitLoop commented Jan 11, 2024

benwis commented Jan 11, 2024 via email

gbj commented Jan 19, 2024 •

edited

Loading

Better handling for trailing slashes. (#2154) #2172

Better handling for trailing slashes. (#2154) #2172

Conversation

NfNitLoop commented Jan 8, 2024

NfNitLoop commented Jan 8, 2024 • edited Loading

NfNitLoop commented Jan 8, 2024 • edited Loading

rakshith-ravi commented Jan 8, 2024 • edited Loading

NfNitLoop commented Jan 8, 2024

rakshith-ravi commented Jan 8, 2024

NfNitLoop commented Jan 8, 2024

benwis commented Jan 8, 2024 • edited Loading

benwis commented Jan 8, 2024

NfNitLoop commented Jan 9, 2024 • edited Loading

NfNitLoop commented Jan 9, 2024

benwis commented Jan 9, 2024

gbj commented Jan 9, 2024

NfNitLoop commented Jan 10, 2024 • edited Loading

NfNitLoop commented Jan 11, 2024

benwis commented Jan 11, 2024

NfNitLoop commented Jan 11, 2024

NfNitLoop commented Jan 11, 2024

benwis commented Jan 11, 2024 via email

gbj commented Jan 19, 2024 • edited Loading

NfNitLoop commented Jan 8, 2024 •

edited

Loading

NfNitLoop commented Jan 8, 2024 •

edited

Loading

rakshith-ravi commented Jan 8, 2024 •

edited

Loading

benwis commented Jan 8, 2024 •

edited

Loading

NfNitLoop commented Jan 9, 2024 •

edited

Loading

NfNitLoop commented Jan 10, 2024 •

edited

Loading

gbj commented Jan 19, 2024 •

edited

Loading