-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat: typed route ids #13864
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
base: main
Are you sure you want to change the base?
feat: typed route ids #13864
Conversation
🦋 Changeset detectedLatest commit: 22b26dc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Haha noice! Would you consider two more things ?
export const params = {
limit: 11,
search: ''
}
|
Typing search params would be great but I think that's a separate issue and it shouldn't hold up this PR. As for |
had to bump the test timeout quite a bit, might be worth running unit tests in a separate workflow |
Perfect to have a follow-up PR for searchParams, yeah, let's do this step by step ;) I like to have a common way to manage ALL links (internal & external). <!-- 🤞 before, hardcoded string, error prone -->
<a href="https://bsky.app/profile/jyc.dev">Bluesky</a>
<!-- ✅ after, typechecked route, no more errors -->
<a href={route('bluesky', { handle: 'jyc.dev' })}>Bluesky</a> Today, I just need to feed the config like this: (similar path structure to "create" params) plugins: [
// ...
kitRoutes({
LINKS: {
bluesky: 'https://bsky.app/profile/[handle]',
}
}),
], Of course, it can be a separate issue to think a bit how to feed this info. But in general, what's your feeling ?
|
Not tonight, but let me know where/how I can contribute to this feature. |
@@ -7,7 +7,8 @@ | |||
"target": "es2022", | |||
"module": "node16", | |||
"moduleResolution": "node16", | |||
"baseUrl": "." | |||
"baseUrl": ".", | |||
"skipLibCheck": true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need skipLibCheck
? I've tried running pnpm check
without it and didn't bump into any issues for the adapters.
EDIT: Alright, I'm seeing the errors now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking of looking into dts-buddy
to see if we can get it to keep the // @ts-ignore
comments in the generated types so that we don't need skipLibCheck
here. It seems like the best way to tell TypeScript "this type doesn't exist yet but will be generated" until something like microsoft/TypeScript#31894 comes along. Do you think this is worth looking into?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth a shot I guess, though dts-buddy is operating on emitted declaration files and I'm pretty sure the comments are lost by that point - might be tricky to correctly recover them from the source
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I managed to get it working in Rich-Harris/dts-buddy#110 . This should remove the need for the hack in generate-dts.js
to preserve the @ts-ignore
comment.
The ts-ignore is only preserved if it meets these two rules:
- It has to be a multi-line comment
/** @ts-ignore ... */
so that it gets recognised as jsdoc - It has to be above the start of a statement instead of in the middle of it such as in line 26 here https://github.com/sveltejs/kit/pull/13864/files#diff-a174e22e6d675073a963705b97687d42e219c2e05eb1cd6d5811c2581d416a8e
For external links: I could imagine a future addition like this, which should be all that's needed from a configuration perspective: // svelte.config.js
export default {
kit: {
paths: {
external: {
bluesky: 'https://bsky.app/profile/[handle]'
}
}
}
}; |
Just realised that |
Another thought: #13840 proposes a You might of course want to do this... resolve(`/blog/${slug}`) ...instead of this: resolveRoute('/blog/[slug]', { slug }) It would just be nice if there was a way to avoid having two very similar APIs. Especially since for kit-routes users, One possibility is for ![]() Is that too loosey-goosey, or does it feel good to people? Personally I like it. Would be great to solve this and deal with #13840 in one go. |
The diff that makes that work, FWIW (obviously we would need to keep diff --git a/packages/kit/src/runtime/app/paths/types.d.ts b/packages/kit/src/runtime/app/paths/types.d.ts
index a9fdfb1f5..f783a4950 100644
--- a/packages/kit/src/runtime/app/paths/types.d.ts
+++ b/packages/kit/src/runtime/app/paths/types.d.ts
@@ -1,5 +1,5 @@
// @ts-ignore
-import { RouteId, RouteParams } from '$app/types';
+import { RouteId, RouteParams, Pathname } from '$app/types';
/**
* A string that matches [`config.kit.paths.base`](https://svelte.dev/docs/kit/configuration#paths).
@@ -15,8 +15,11 @@ export let base: '' | `/${string}`;
*/
export let assets: '' | `https://${string}` | `http://${string}` | '/_svelte_kit_assets';
-type ResolveRouteArgs<T extends RouteId> =
- RouteParams<T> extends Record<string, never> ? [route: T] : [route: T, params: RouteParams<T>];
+type ResolveRouteArgs<T extends RouteId | Pathname> = T extends RouteId
+ ? RouteParams<T> extends Record<string, never>
+ ? [route: T]
+ : [route: T, params: RouteParams<T>]
+ : [route: T];
/**
* Populate a route ID with params to resolve a pathname.
@@ -33,4 +36,4 @@ type ResolveRouteArgs<T extends RouteId> =
* ); // `/blog/hello-world/something/else`
* ```
*/
-export function resolveRoute<T extends RouteId>(...args: ResolveRouteArgs<T>): string;
+export function resolve<T extends RouteId | Pathname>(...args: ResolveRouteArgs<T>): string; |
I would like to check a few points from
// format: route(path) -> default <-
route("/site/[id]", { id: 7, tab: 'info' })
// format: route(symbol)
route("site_id", { id: 7, tab: 'info' })
// format: `variables` (best for code splitting & privacy)
PAGE_site_id({ id: 7, tab: 'info' })
// format: object[path]
PAGES["/site/[id]"]({ id: 7, tab: 'info' })
// format: object[symbol]
PAGES.site_id({ id: 7, tab: 'info' }) But I belive that it's not needed,
route('/[[lang]]/one') // ❌ Doesn't exist.
route('/one', { lang: 'fr' }) // ✅ valid path (lang is optional)
route('/(gp)/two') // ❌ Doesn't exist.
route('/two') // ✅ valid path
'/site/[id]': {
params: {
id: { type: 'string', default: 'Vienna' },
lang: { type: "'fr' | 'hu' | undefined", default: 'fr' },
},
}, So with
retour('/match/[id=int]', { id: 2 })
// id: ExtractParamType<typeof import('../params/int.ts').match The support is not so good.
export const searchParams = {
limit: 11,
search: ''
}
// svelte.config.js
export default {
kit: {
paths: {
external: {
bluesky: 'https://bsky.app/profile/[handle]'
}
}
}
};
'/anchors': {
hash: {
type: '"section0" | "section1" | "section2" | "section3"',
required: true,
default: 'section0',
},
}, Could maybe be a settings in
<a href={route('/a/[...rest]/z', { rest: ['SWAGER', 'GRAPHIQL'] })}>Rest SWAGER GRAPHIQL</a>
<a href={route('/[x+2e]well-known')}>Well Known</a>
<a href={route('/[u+d83e][u+dd2a]')}>🤪</a>
<a href={route('/[u+d83e][u+dd2a]/[emoji]/[u+2b50]', { emoji: '🚀' })}>🤪🚀⭐</a>
'GET /site': (params?: { lang?: 'fr' | 'en' | 'hu' | 'at' | string }) => {
return `${params?.['lang'] ? `/${params?.['lang']}` : ''}/site`
},
'default /contract/[id]': (params: {
id: string | number
lang?: 'fr' | 'en' | 'hu' | 'at' | string
limit?: number
}) => {
return `${params?.['lang'] ? `/${params?.['lang']}` : ''}/contract/${params['id']}${appendSp({ limit: params['limit'] })}`
},
'create /site': (params?: {
lang?: 'fr' | 'en' | 'hu' | 'at' | string
redirectTo?: 'list' | 'new' | 'detail'
}) => {
return `${params?.['lang'] ? `/${params?.['lang']}` : ''}/site?/create${appendSp({ redirectTo: params?.['redirectTo'] }, '&')}`
},
resolve('/blog/[slug]', { slug })
resolve(`/blog/${slug}`) We don't HAVE TO support everything from the start... But it's maybe a good checklist to see what we want right now, later and never. Tomorrow, I could add some tests around all this? (in a PR targeting this PR) |
Yes, you can do this (substitute whichever package manager you're using for pnpm i -D https://pkg.pr.new/@sveltejs/kit@13864 |
Annoying discovery: TypeScript collapses e.g. You do still get type safety, but it definitely feels less magical than getting autocomplete for all your route IDs. I can't decide what the best trade-offs are here. |
Added the ![]() Also deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, few nits
<div class="ts-block"> | ||
|
||
```dts | ||
type RouteParams<T extends RouteId> = { /* generated */ } | Record<string, never>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a way this could be more useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
such as?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/* generated */
doesn't really help me understand what's actually going on here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps an example with a concrete type param passed would be helpful? Same for the one below
|
||
// this is hacky as all hell but it gets the tests passing. might be a bug in dts-buddy? | ||
// prettier-ignore | ||
writeFileSync('./types/index.d.ts', types.replace("declare module '$app/server' {", `declare module '$app/server' { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:wat:
|
||
dynamic_routes.push(route_type); | ||
|
||
pathnames.push( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as a general rule, could we pull out regexes like this into util functions that say what they do? Like... I'm pretty sure this is removing all square brackets but it is really hard to read, and there are a bunch of similar-but-different ones in the file
Co-authored-by: Rich Harris <[email protected]>
Co-authored-by: Rich Harris <[email protected]>
#13881 is a PR against this branch that adds additional tests |
import { s } from '../../../utils/misc.js'; | ||
|
||
const remove_relative_parent_traversals = (/** @type {string} */ path) => | ||
path.replace(/\.\.\//g, ''); |
Check failure
Code scanning / CodeQL
Incomplete multi-character sanitization High
../
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
To fix the issue, the regular expression replacement should be applied repeatedly until no more instances of ../
remain in the string. This ensures complete sanitization of the input. The best approach is to use a do...while
loop to repeatedly apply the replacement until the input string stabilizes (i.e., no further replacements occur). This fix is localized to the remove_relative_parent_traversals
function and does not alter its intended functionality.
-
Copy modified lines R10-R17
@@ -9,4 +9,10 @@ | ||
|
||
const remove_relative_parent_traversals = (/** @type {string} */ path) => | ||
path.replace(/\.\.\//g, ''); | ||
const remove_relative_parent_traversals = (/** @type {string} */ path) => { | ||
let previous; | ||
do { | ||
previous = path; | ||
path = path.replace(/\.\.\//g, ''); | ||
} while (path !== previous); | ||
return path; | ||
}; | ||
const replace_optional_params = (/** @type {string} */ id) => |
resolves #11386
This is long overdue, but @jycouet goaded me into working on it with the 1.0 release of
vite-plugin-kit-routes
🎉Everyone should have type safety when dealing with routes!
This PR adds a new generated
$app/types
module which exports three types:RouteId
is a union of all the routes in your app ('/' | '/about' | '/blog' | '/blog/[slug]'
etc)RouteParams<T extends RouteId>
is a utility that gives you the type of the params for a given routeLayoutParams<T extends RouteId>
is the same, but also gives you the type of any children ofT
(for example, if you're in the/blog
layout, the route could be either/blog
or/blog/[slug]
, soslug
is an optional paramMost of the time you won't use these types directly, but via things like
page.route.id
andresolveRoute
:I couldn't figure out a sensible way to add a test for this. Maybe someone else can, if not then I'm not too worried about it.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.Edits