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

feat(react-router): allow unencoded characters in dynamic path segments #2677

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

alma-lp
Copy link
Contributor

@alma-lp alma-lp commented Oct 30, 2024

Currently any path params with a "@" character are URI encoded. Modern browsers handle this fine and it is a common pattern (e.g. youtube.com/@exampleChannel) to have them unencoded in a path.

Copy link

nx-cloud bot commented Oct 31, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit e21f904. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

Copy link

pkg-pr-new bot commented Oct 31, 2024

Open in Stackblitz

More templates

@tanstack/create-router

pnpm add https://pkg.pr.new/@tanstack/create-router@2677

@tanstack/eslint-plugin-router

pnpm add https://pkg.pr.new/@tanstack/eslint-plugin-router@2677

@tanstack/react-cross-context

pnpm add https://pkg.pr.new/@tanstack/react-cross-context@2677

@tanstack/history

pnpm add https://pkg.pr.new/@tanstack/history@2677

@tanstack/react-router

pnpm add https://pkg.pr.new/@tanstack/react-router@2677

@tanstack/react-router-with-query

pnpm add https://pkg.pr.new/@tanstack/react-router-with-query@2677

@tanstack/router-arktype-adapter

pnpm add https://pkg.pr.new/@tanstack/router-arktype-adapter@2677

@tanstack/router-cli

pnpm add https://pkg.pr.new/@tanstack/router-cli@2677

@tanstack/router-devtools

pnpm add https://pkg.pr.new/@tanstack/router-devtools@2677

@tanstack/router-generator

pnpm add https://pkg.pr.new/@tanstack/router-generator@2677

@tanstack/router-plugin

pnpm add https://pkg.pr.new/@tanstack/router-plugin@2677

@tanstack/router-valibot-adapter

pnpm add https://pkg.pr.new/@tanstack/router-valibot-adapter@2677

@tanstack/router-vite-plugin

pnpm add https://pkg.pr.new/@tanstack/router-vite-plugin@2677

@tanstack/router-zod-adapter

pnpm add https://pkg.pr.new/@tanstack/router-zod-adapter@2677

@tanstack/start

pnpm add https://pkg.pr.new/@tanstack/start@2677

@tanstack/start-vite-plugin

pnpm add https://pkg.pr.new/@tanstack/start-vite-plugin@2677

@tanstack/virtual-file-routes

pnpm add https://pkg.pr.new/@tanstack/virtual-file-routes@2677

commit: e21f904

packages/react-router/src/path.ts Outdated Show resolved Hide resolved
@schiller-manuel
Copy link
Contributor

should we maybe expose this as a config option to the user? some might want different characters, some might want no characters etc?

@SeanCassiere
Copy link
Member

should we maybe expose this as a config option to the user? some might want different characters, some might want no characters etc?

That's a good idea!

@alma-lp would this be something you'd be able to implement?

@alma-lp
Copy link
Contributor Author

alma-lp commented Nov 3, 2024

should we maybe expose this as a config option to the user? some might want different characters, some might want no characters etc?

That's a good idea!

@alma-lp would this be something you'd be able to implement?

I should. I'll take a look.

@schiller-manuel
Copy link
Contributor

please add a test that configures router with some allowed chars, navigate to a route by setting a path param to contain one of the allowed chars and then check if the url is not encoded

@CanRau
Copy link
Contributor

CanRau commented Nov 4, 2024

Interesting. this is a completely different approach, but seemingly much simpler, than adding prefix/suffix support, tho if I understand it correctly would actually be sufficient for my usecase as we use /@username routes as well.

Just for reference, I added some thoughts here #707 (comment)

@schiller-manuel
Copy link
Contributor

schiller-manuel commented Nov 4, 2024

@CanRau this does not allow prefixes. this just prevents path params containing e.g. "@" to be urlencoded. the path param will still be the full string @foo

@CanRau
Copy link
Contributor

CanRau commented Nov 4, 2024

Yea just realized that it wouldn't help with route matching 😔 which we need as we have those /@username paths at the root next to other dynamic and non-dynamic routes 😬

@SeanCassiere

This comment was marked as resolved.

@SeanCassiere SeanCassiere changed the title feat(react-router): allow @ characters in path segments feat(react-router): allow unencoded characters in dynamic path segments Nov 5, 2024
@SeanCassiere SeanCassiere merged commit cc05ad8 into TanStack:main Nov 5, 2024
5 checks passed
@jmkd3v
Copy link

jmkd3v commented Nov 28, 2024

It looks like this fix is incomplete. Links to routes such as <Link to="/$handle" params={{ handle: "@hello" }} /> still direct to /%40handle. Something like <Link to="/@hello" /> where the url-encoded character is in the to prop itself directs to the correct page, but my understanding is that this is not the correct approach.

@CanRau
Copy link
Contributor

CanRau commented Nov 28, 2024

@jmkd3v have you configured allowed characters on your router?

const router = createRouter({
  ...
  pathParamsAllowedCharacters: ['@']
})

@jmkd3v
Copy link

jmkd3v commented Nov 28, 2024

oh, I didn't know I had to configure it! that fixed it, thank you so much @CanRau!

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.

5 participants