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

Support square brackets for advanced dynamic path segments (prefix, suffix & multiple) #2734

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

CanRau
Copy link
Contributor

@CanRau CanRau commented Nov 11, 2024

Hey, this is my take so far on #707 inspired by #709

Also tried my luck making it type safe, mostly ChatGPT tho and I'm actually not completely sure it works 😬 but I think it's a good enough start to open a PR, as I'd love prefixes to be officially supported 😇

I'm very curious what the team thinks about this approach, for explanation, I decided to not stick to $ because having start [ and end ] properly delimited make suffixes and even multiple dynamic params easier or even possible. Also as mentioned before in #707 (comment) SvelteKit, Rakkas and possibly others already use square brackets, tho I'm curious if there's potential downsides?

So this PR should make the following possible:
prefix: @[username]
suffix: [username]@
both: @[username]@
where @ could of course be anything
multiple: [firstname]-[lastname]

What I think might be nice for multiple params, would be better matching so that it could properly distinguish/handle [firstname]-[lastname] and [firstname]-[middlename]-[lastname] which it doesn't yet, so currently the first route would also handle the second and the second would never match. 😅 should be fixed now

Closes: #707

Copy link

nx-cloud bot commented Nov 11, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 77bf7fb. 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


🟥 Failed Commands
nx affected --targets=test:eslint,test:unit,test:e2e,test:types,test:build,build --parallel=3
✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

Copy link

pkg-pr-new bot commented Nov 11, 2024

Open in Stackblitz

More templates

@tanstack/create-router

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

@tanstack/eslint-plugin-router

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

@tanstack/react-cross-context

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

@tanstack/history

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

@tanstack/react-router

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

@tanstack/react-router-with-query

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

@tanstack/router-arktype-adapter

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

@tanstack/router-cli

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

@tanstack/router-devtools

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

@tanstack/router-generator

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

@tanstack/router-plugin

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

@tanstack/router-valibot-adapter

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

@tanstack/router-vite-plugin

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

@tanstack/router-zod-adapter

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

@tanstack/start

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

@tanstack/start-vite-plugin

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

@tanstack/virtual-file-routes

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

commit: 77bf7fb

@CanRau CanRau marked this pull request as draft November 11, 2024 16:17
@schiller-manuel
Copy link
Contributor

As we will also add optional path params we must check whether this can be decoupled or not, especially when it comes to types. @chorobin let's discuss this

@CanRau
Copy link
Contributor Author

CanRau commented Nov 11, 2024

@schiller-manuel interesting I see 🤔 yea would of course be nice if everything would work properly together 🤞
for when do you have optional params planned?

just pushed an improvement so routes should be better sorted now to address the last part of my initial comment.

Also not sure breaking routing completely is desirable 😅 tho would it be helpful to drop $param for params and only use square brackets [param] for everything? Or find an alternative to square brackets 🤔 to not have to support 2 different ways of defining dynamic params

@schiller-manuel
Copy link
Contributor

we will definitely not introduce a breaking API change in v1 by removing support for $

@CanRau
Copy link
Contributor Author

CanRau commented Nov 15, 2024

Yea totally understand just wanted to've mentioned it 😇
Currently patching this in via pnpm patch 🤓

@CanRau
Copy link
Contributor Author

CanRau commented Nov 16, 2024

What could I do to potentially help move this forward?
If it's about optional params? I guess no one is currently working on them right?

@schiller-manuel
Copy link
Contributor

first, we need to define the full set of features we want to support with path params (and maybe other related features that would be influenced by it such as parallel routes ) so we can solve this holistically.

so you could open up an RFC style discussion to get this started.

@CanRau
Copy link
Contributor Author

CanRau commented Nov 19, 2024

Opened the RFC as WIP to get started #2800

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.

Support for static prefix on dynamic paths
2 participants