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.

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

router: Retain trailing slashes in paths #2163

Closed
wants to merge 1 commit into from

Conversation

skirsdeda
Copy link
Contributor

This addresses client-side part of #2154 leaving path matching trailing slash insensitive (to be compatible with current behavior). For example /*path/ would still match both foo and foo/. But when clicking on a link foo/, trailing slash would be retained.

I think it's sensible to keep matching compatible until server-side for this is figured out.

Since path resolution implementation in router is copied from solidjs router and that one retains trailing slashes, I just fixed regexp recreations in rust code to fully match ones from solidjs router:
https://github.com/solidjs/solid-router/blob/main/src/utils.ts#L5:

const trimPathRegex = /^\/+|(\/)\/+$/g;
//
export function normalizePath(path: string, omitSlash: boolean = false) {
  const s = path.replace(trimPathRegex, "$1");
  //
}

(note that trailing slash is retained (match group $1) but excessive trailing slashes are trimmed)
https://github.com/solidjs/solid-router/blob/main/src/utils.ts#L37:

export function joinPaths(from: string, to: string): string {
  return normalizePath(from).replace(/\/*(\*.*)?$/g, "") + normalizePath(to);
}

(matches wildcard pattern optionally from right and removes it along with all trailing slashes in from path)

@NfNitLoop
Copy link
Contributor

NfNitLoop commented Jan 7, 2024

Testing this locally, with:

    let paths = generate_route_list(leptos_app::App);
    dbg!(&paths);
    cfg.leptos_routes(options.clone(), paths, leptos_app::App);
    cfg.app_data(Data::new(options));

I still see routes with their trailing slashes trimmed. I bet this is leptos-actix. I'm forking this branch and adding on some changes to leptos-actix as well to confirm. 🤞


(Update) Nope, looks like the RouteListing items are coming out of leptos_router generate_route_list_inner_with_context() without their trailing slashes. (I'm still looking into things. Just sharing details in case I get distracted before completing things. 😅)

@skirsdeda
Copy link
Contributor Author

@NfNitLoop
Copy link
Contributor

try removing this line

That did the trick! I think we should not trim there, because that path gets returned by leptos_router::generate_route_list_inner() to the integrations. Without the trailing / you can't know the user's intent.

The good news is that after removing that line, the leptos-actix integration does the "right thing" (IMO) and also keeps the trailing slash. I wrote some unit tests to show this behavior at the two layers it passes through:

https://github.com/skirsdeda/leptos/compare/trailing_slashes...NfNitLoop:leptos:trailing_slashes?expand=1

leaving path matching trailing slash insensitive

This has an unfortunate side-effect. Now with <Route path="/foo/">,

  • /foo on the server gives a 404.
  • /foo/ on the server works.
  • After hydration, /foo on the client does not give a 404. 🤔

IMO you really want your client and server behavior to match exactly or you'll end up with a lot of headaches.

I'm going to continue learning the codebase and see how I can make them align. May also play around w/ some ideas in #2154 on top of this branch. 😊

@skirsdeda
Copy link
Contributor Author

Closing this in favor of #2172.

@skirsdeda skirsdeda closed this Jan 8, 2024
@NfNitLoop
Copy link
Contributor

Thanks for this work, though! It was a good start for building the full feature, and a primer for me for diving into the Leptos codebase!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants