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

location.pathname is not encoded on client side navigation #1441

Closed
hi-ogawa opened this issue Apr 8, 2024 · 0 comments · Fixed by #1490
Closed

location.pathname is not encoded on client side navigation #1441

hi-ogawa opened this issue Apr 8, 2024 · 0 comments · Fixed by #1490
Labels
bug Something isn't working

Comments

@hi-ogawa
Copy link

hi-ogawa commented Apr 8, 2024

Describe the bug

After clicking a link with a special character such as <Link to="/repro/$id" params={{ id: "✅" }} />, router's location state useRouterState({ select: s => s.location.pathname }) has non-encoded value /repro/✅.

This behavior might not be ideal since browser's actual location state is always encoded /repro/%E2%9C%85. For example, as illustrated in the reproduction below, simple browser reload will give an encoded pathname from useRouterState and this will make Link.activeProps to miss the original link before the reload.

Your Example Website or App

https://github.com/hi-ogawa/tanstack-router/tree/fix-pathname-encoding/examples/react/basic-ssr-file-based

Steps to Reproduce the Bug or Issue

  1. Click "Repro 1" link
  2. Click browser reload button and see link active state changes

The same can be reproduced on stackblitz https://github.com/hi-ogawa/tanstack-router/tree/fix-pathname-encoding/examples/react/basic-ssr-file-based, but their reload button in IDE preview works differently, so you might need to use "Open preview in a new tab" and test there.

Expected behavior

I'm actually not sure what's the expected behavior, but it looks like react-router had a similar issue before. They seem to choose to always encode a pathname from their location state hook. Probably this is a relevant PR:

I haven't actually tested with react router, but what the author wrote in the PR description makes sense to me as an expected behavior.

Screenshots or Videos

2024-04-08.11-59-31.webm

Platform

  • OS: Linux
  • Browser: Chrome
  • Version: 1.26.12 (main branch when this issue is created)

Additional context

On my react server framework https://github.com/hi-ogawa/vite-plugins/tree/main/packages/react-server, I'm using @tanstack/history for client side history management. I was trying to implement something like @tanstack/router's Link with activeProps in one of my demo hi-ogawa/react-server-demo-remix-tutorial#3. While doing that, I discovered this slight inconsistency and I found that the same issue can be reproduced on @tanstack/router, so I thought I'd report an issue here as well.

EDIT: My comment from here turns out to be irrelevant (see #1442 (comment)), so I'll hide it. The solution I ended up is to patch history.push/replace so that history only sees encoded version of href hi-ogawa/vite-plugins#276.

(open old comment)

Maybe the fix (if necessary) should be done in router integration and not in @tanstack/history itself since it provides createHref as an customization point. In my case, I plan to do something like this when doing createBrowserHistory:

  const history = createBrowserHistory({
    createHref(path) {
      // consistently return encoded url as history state.
      // i.e. history.push("/✅") should set { pathname: "/%E2%9C%85" } as state
      const url = new URL(path, window.location.origin);
      const newPath = url.href.slice(url.origin.length);
      return newPath;
    },
  });

But, I found that currently custom createHref doesn't reach history location object (note parseHref(destHref, ...) instead of parseHref(href, ...) below), so for now, I patched this part of the code. I wasn't sure if this is a bug of @tanstack/history, but if it looks like so, I'll probably open a separate issue for this later.

const href = createHref(destHref)
if (!scheduled) {
rollbackLocation = currentLocation
}
// Update the location in memory
currentLocation = parseHref(destHref, state)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants