Skip to content

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

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.

Sign up for GitHub

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 12 commits into from
Oct 12, 2023
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1d835fc
Use a matcher's type guard for param types if present
LorisSigrist Sep 19, 2023
fcb6c31
Run prettier
LorisSigrist Sep 20, 2023
8ba729e
Add changeset
LorisSigrist Sep 20, 2023
9aceaa7
Fix linting error
LorisSigrist Sep 20, 2023
35beecf
Lowercase comment
LorisSigrist Sep 21, 2023
48ad59e
Lowercase Comment
LorisSigrist Sep 21, 2023
385e175
Add param type inference tests
LorisSigrist Oct 10, 2023
6f34096
check + format + lint
LorisSigrist Oct 10, 2023
ee11e6f
Disable linter in type-tests
LorisSigrist Oct 10, 2023
a8d399e
Use @ts-expect-error instead of @ts-ignore
LorisSigrist Oct 10, 2023
abcc708
Implement Feedback
LorisSigrist Oct 10, 2023
b7ade00
Update packages/kit/src/core/sync/write_types/index.js
dummdidumm Oct 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/great-bags-heal.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

feat: infer route parameter type from matcher's guard check if applicable
43 changes: 35 additions & 8 deletions packages/kit/src/core/sync/write_types/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,10 +188,14 @@ function update_types(config, routes, route, to_delete = new Set()) {
// add 'Expand' helper
// Makes sure a type is "repackaged" and therefore more readable
declarations.push('type Expand<T> = T extends infer O ? { [K in keyof O]: O[K] } : never;');

//Returns the predicate of a matcher's type guard - or string if there is no type guard
declarations.push(
`type RouteParams = { ${route.params
.map((param) => `${param.name}${param.optional ? '?' : ''}: string`)
.join('; ')} }`
'//@ts-ignore\ntype MatcherParam<M> = M extends (param: string) => param is infer U ? U : string;'
Copy link
Member

@dummdidumm dummdidumm Oct 10, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@LorisSigrist LorisSigrist Oct 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@benmccann benmccann Oct 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@LorisSigrist LorisSigrist Oct 10, 2023

Choose a reason for hiding this comment

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

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

Copy link
Member

@dummdidumm dummdidumm Oct 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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"

);

declarations.push(
'type RouteParams = ' + generate_params_type(route.params, outdir, config) + ';'
);

if (route.params.length > 0) {
Expand Down Expand Up @@ -265,7 +269,8 @@ function update_types(config, routes, route, to_delete = new Set()) {

if (route.layout) {
let all_pages_have_load = true;
const layout_params = new Set();
/** @type {import('types').RouteParam[]} */
const layout_params = [];
const ids = ['RouteId'];

route.layout.child_pages?.forEach((page) => {
Expand All @@ -274,7 +279,9 @@ function update_types(config, routes, route, to_delete = new Set()) {
if (leaf.route.page) ids.push(`"${leaf.route.id}"`);

for (const param of leaf.route.params) {
layout_params.add(param.name);
//Skip if already added
if (layout_params.some((p) => p.name === param.name)) continue;
layout_params.push({ ...param, optional: true });
}

ensureProxies(page, leaf.proxies);
Expand All @@ -301,9 +308,7 @@ function update_types(config, routes, route, to_delete = new Set()) {
declarations.push(`type LayoutRouteId = ${ids.join(' | ')}`);

declarations.push(
`type LayoutParams = RouteParams & { ${Array.from(layout_params).map(
(param) => `${param}?: string`
)} }`
'type LayoutParams = RouteParams & ' + generate_params_type(layout_params, outdir, config)
);

const {
Expand Down Expand Up @@ -567,6 +572,28 @@ function replace_ext_with_js(file_path) {
return file_path.slice(0, -ext.length) + '.js';
}

/**
* @param {import('types').RouteParam[]} params
* @param {string} outdir
* @param {import('types').ValidatedConfig} config
*/
function generate_params_type(params, outdir, config) {
/** @param {string} matcher */
const path_to_matcher = (matcher) =>
posixify(path.relative(outdir, path.join(config.kit.files.params, matcher)));

return `{ ${params
.map(
(param) =>
`${param.name}${param.optional ? '?' : ''}: ${
param.matcher
? `MatcherParam<typeof import('${path_to_matcher(param.matcher)}').match>`
: 'string'
}`
)
.join('; ')} }`;
}

/**
* @param {string} content
* @param {boolean} is_server
Expand Down