Infer route parameter type from matcher's guard check if applicable

[LorisSigrist]

closes #8137 - Just not for $page.params

Currently all route parameters always have type string. This type is correct, but often broader than it needs to be.
If a matcher is used on a route parameter, and that matcher is annotated with a guard check, we can infer the type much more narrowly.

// src/params/ab.ts
export const match = (param) : param is "a" | "b" => param == "a" || param == "b";

// src/routes/[myParam=ab]/+page.ts
export async function load({params}) {
    params.myParam //Has type "a" | "b"
}

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:.

[changeset-bot]

🦋 Changeset detected

Latest commit: b7ade00

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	Patch

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

[dummdidumm]

Could you add a test here?

[dummdidumm]

Why do we need to @ts-ignore this? Also, could this theoretically result in the wrong type if someone creates a param matcher which says param is number, thinking they could narrow the type to something else than a string-like?

[LorisSigrist]

Both questions have an answer stemming from the following behaviour:

Typescript will complain if a predicate is not assignable to the type of the parameter. For example:

const matcher = (param: string) : param is 5 => //...

would error since typeof 5 is not assignable to type string.

Since SvelteKit always types the parameter of a matcher as a string it shouldn't be possible to write a non-string predicate on a matcher.

As for the @ts-ignore, try using the MatcherParam type without it: (TS Playground Link)

type MatcherParam<M> = M extends (param: string) => param is infer U ? U : string;

Typescript believes that the type of infer U could be anything and therefore isn't assignable to type string. This is clearly wrong. The infer U should have been type-narrowed to string because param is typed as string. The predicate checking I described above prevents any other case.

If you really don't like the @ts-ignore, we could change the type to:

type MatcherParam<M> = M extends (param: any /* <- */) => param is infer U ? U : string;

This would still work, but I prefer specifying that param is a string, since the fallback (the : string at the end) is a string.

[benmccann]

I'll defer to @dummdidumm on the question of whether we should ignore the TS issue, but if we want to it sounds like we should use @ts-expect-error. We use @ts-ignore to signal a to-do that needs to be cleaned up and @ts-expect-error where we plan to leave it in that state permanently

[LorisSigrist]

Makes sense. I changed it to @ts-expect-error. Thanks for taking the time!

[dummdidumm]

Can we make the type

type MatchParams<M> = M extends (param : string) => param is infer U ? U extends string ? U : string : string;

That way if someone writes a matcher doing param is number (which is wrong, but they might anyway) the type is still string and not switched to number.

Also, can you switch the @ts-expect-error back to @ts-ignore in this instance? I don't wanna risk TS changing its mind somewhere down the line that this should no longer error (to me this is different to the other @ts-expect-error as the "remove this when TS no longer errors" mostly applies to internal code, not user-facing code). Plus, can you add a small comment explaining why we need the ignore? Something along the lines of "TS errors on infer U, which seems weird"

[dummdidumm]

Thank you!

[benmccann]

Thanks for the review @dummdidumm ! I'm going to merge this based on your approval