Skip to content

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

Merged
merged 38 commits into from
Jul 24, 2025
Merged

feat: typed route ids #13864

merged 38 commits into from
Jul 24, 2025

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Jun 7, 2025

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 route
  • LayoutParams<T extends RouteId> is the same, but also gives you the type of any children of T (for example, if you're in the /blog layout, the route could be either /blog or /blog/[slug], so slug is an optional param

Most of the time you won't use these types directly, but via things like page.route.id and resolveRoute:

Screenshot 2025-06-07 at 3 49 11 PM

Screenshot 2025-06-07 at 3 50 53 PM

Screenshot 2025-06-07 at 3 49 32 PM

Screenshot 2025-06-07 at 3 49 51 PM

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:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

  • Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

Copy link

changeset-bot bot commented Jun 7, 2025

🦋 Changeset detected

Latest commit: de26113

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Minor

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

@svelte-docs-bot
Copy link

@jycouet
Copy link
Contributor

jycouet commented Jun 7, 2025

Haha noice!
Let's link to: #11386

Would you consider two more things ?

  • 1/ being able to specify searchParams in like +page.ts
export const params = {
  limit: 11,
  search: ''
}
  • 2/ How to add things in the list of resolveRoute ? To manage external links ?

@Rich-Harris
Copy link
Member Author

Typing search params would be great but I think that's a separate issue and it shouldn't hold up this PR. As for resolveRoute, I'm not sure what you mean — it's only meant to be used with the routes of your app, why would you need to do stuff with external links?

@Rich-Harris
Copy link
Member Author

had to bump the test timeout quite a bit, might be worth running unit tests in a separate workflow

@jycouet
Copy link
Contributor

jycouet commented Jun 7, 2025

Typing search params would be great but I think that's a separate issue and it shouldn't hold up this PR. As for resolveRoute, I'm not sure what you mean — it's only meant to be used with the routes of your app, why would you need to do stuff with external links?

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 ?


vite-plugin-kit-routes DEPRECATED s00n 🎉

@jycouet
Copy link
Contributor

jycouet commented Jun 7, 2025

had to bump the test timeout quite a bit, might be worth running unit tests in a separate workflow

Not tonight, but let me know where/how I can contribute to this feature.

@eltigerchino eltigerchino added the feature / enhancement New feature or request label Jun 9, 2025
@@ -7,7 +7,8 @@
"target": "es2022",
"module": "node16",
"moduleResolution": "node16",
"baseUrl": "."
"baseUrl": ".",
"skipLibCheck": true
Copy link
Member

@eltigerchino eltigerchino Jun 9, 2025

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.

Copy link
Member

@eltigerchino eltigerchino Jun 9, 2025

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?

Copy link
Member Author

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

Copy link
Member

@eltigerchino eltigerchino Jun 20, 2025

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 sveltejs/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:

  1. It has to be a multi-line comment /** @ts-ignore ... */ so that it gets recognised as jsdoc
  2. 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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah nice. I don't have time to review that PR right now but I left a comment over there to remind myself to come back and clean this up later

@Rich-Harris
Copy link
Member Author

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]'
      }
    }
  }
};

@Rich-Harris
Copy link
Member Author

Just realised that resolveRoute('/foo') errors because of a missing second argument, which is the opposite of what's supposed to happen. Investigating

Co-authored-by: Rich Harris <richard.a.harris@gmail.com>
Co-authored-by: Rich Harris <richard.a.harris@gmail.com>
@benmccann
Copy link
Member

#13881 is a PR against this branch that adds additional tests

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this only get docs on types? I couldn't figure out anywhere else we reference it.

@Rich-Harris Rich-Harris merged commit a8cc450 into main Jul 24, 2025
21 checks passed
@Rich-Harris Rich-Harris deleted the typed-route-ids branch July 24, 2025 15:43
@github-actions github-actions bot mentioned this pull request Jul 24, 2025
@charbelnicolas
Copy link

The following code no longer works when using resolve() for href and export const trailingSlash = 'always';

<Link href={resolve('/dashboard/settings/')} label="SETTINGS" />

link.svelte

<script>

	import { page } from '$app/stores';

	let { href, label } = $props();

</script>

<a href={href} class="header-button font-medium" class:selected-link={$page.url.pathname === href} aria-label={label}>

	{label}

</a>

$page.url.pathname === href is never the same, one has a trailing slash, the other doesn't.

href = /portal/dashboard/settings and $page.url.pathname = /portal/dashboard/settings/

If using trailingSlash = always, resolve() should add it too IMO.

Please let me know if I'm wrong and should do a work around this issue.

@charbelnicolas
Copy link

The following code no longer works when using resolve() for href and export const trailingSlash = 'always';

<Link href={resolve('/dashboard/settings/')} label="SETTINGS" />

link.svelte

<script>

	import { page } from '$app/stores';

	let { href, label } = $props();

</script>

<a href={href} class="header-button font-medium" class:selected-link={$page.url.pathname === href} aria-label={label}>

	{label}

</a>

$page.url.pathname === href is never the same, one has a trailing slash, the other doesn't.

href = /portal/dashboard/settings and $page.url.pathname = /portal/dashboard/settings/

If using trailingSlash = always, resolve() should add it too IMO.

Please let me know if I'm wrong and should do a work around this issue.

I'm using let hrefWithSlash = $derived(${href}/); as a simple workaround but we shouldn't need any workarounds :)

@Rich-Harris
Copy link
Member Author

Please open an issue rather than commenting on a closed PR, where it will get lost

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature / enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

resolveRoute improvements
7 participants