[breaking] replace routeId with route.id

[dummdidumm]

Closes #6980

Draft, because the tracking part is missing, and I'm not sure yet if we want to replace routeId in all places (like $page.routeId)

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. All changesets should be patch until SvelteKit 1.0

[changeset-bot]

🦋 Changeset detected

Latest commit: 85d15e1

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

[Conduitry]

Is there anything else we foresee living in route? params could get moved in there maybe?

Whatever we do, for consistency I'd be in favor of having $page be the same shape to the extent possible.

[dummdidumm]

params could be moved into it, but that would be too big of a breaking change at this point, and it also results in worse DX, because you'd have to destructure the input twice then.

[Rich-Harris]

The site is failing to deploy because a code snippet in the docs references route.id instead of route?.id. It'd be easy to fix, but I wonder if instead the types should reflect the fact that route always exists in the context of load or a +server.js handler?

[Rich-Harris]

I made that change for LoadEvent and ServerLoadEvent, trying to figure out the best approach for RequestHandler. If I make this change...

export interface RequestHandler<
	Params extends Partial<Record<string, string>> = Partial<Record<string, string>>
> {
-	(event: RequestEvent<Params>): MaybePromise<Response>;
+	(event: RequestEvent<Params> & { route: { id: string } }): MaybePromise<Response>;
}

...then it works, but this is what I get if I hover over route:

Not totally ideal. Do we need to come up with a new type that extends RequestEvent? Or should the route ID be a generic argument? (That way we could make it an actual value for the generated types, or a list of possible values for layouts, etc)

[dummdidumm]

I thought this, too, but in some cases the root layout is loaded but without a route - for example when an infinite redirect loop is detected, or in case of a 404 - many ugh-vibes on this PR 😅 route.id is probably fine most of the time, but one could forget it and it would crash more likely than justing using routeId - an argument in favor of getRouteId()?

[Rich-Harris]

Ah crap, you're right — will revert the last commit.

What if we went the generic route though, and made route always exist but with id defaulting to string | null?

// .svelte-kit/types/src/routes/blog/$types.d.ts
export type PageLoad<...> = Kit.Load<..., OutputData, 'blog'>;
export type LayoutLoad<...> = Kit.Load<..., OutputData, 'blog' | 'blog/[slug]'>;

Or is it misleading for the route object to exist when no route matched?

[dummdidumm]

Funny how we go through the same thought process independently. I asked myself the same question, and came to the conclusion that it's weird. Then again, all of this is weird, so we need to choose something that is the least weird while being easy enough to use. After having implemented this, part of me thinks getRouteId() would be better after all - unless you imagine any other things ending up in that object later on.
The generic is interesting, although I'm wondering if that typing maintenance is worth it.
What are people using routeId for anyway? Surely it's only in layouts?

[Rich-Harris]

And just as I'd come around to route.id! It's definitely more future-proof, but I can't imagine what it would need to be future-proof against (i.e. I can't think of any properties we'd need beyond id). Then again that's sort of the point of being future-proof — you can't anticipate everything.

I think parity with $page is a reasonably convincing argument. We could have event.getRouteId() and $page.routeId but it would definitely feel a bit more natural to have event.route.id and $page.route.id.

What are people using routeId for anyway? Surely it's only in layouts?

Yeah, I think the main use case is where you want some special case logic in a layout ('require the user to be logged in, unless the current routeId is /login', or whatever).

[dummdidumm]

I like route.id more for future-proofness and for how it looks when reading it. I'm not sure how I feel about route: { id: string | null } vs route: { id: string } | null. The latter feels more correct, but it's easier to produce runtime nullpointers with it. So ... yeah, maybe go with route: { id: string | null }

[dummdidumm]

do we need the overhead of that interface on the action interfaces? Actions can only be used on pages, so routeId can only be one thing, so it's useless to use it, so probably noone does that, so we probably can just type this as string on RequestEvent

[Rich-Harris]

Is there any real drawback? It's contrived but you could imagine someone doing this sort of thing, where some_shared_logic is picky about the second argument:

export const actions = {
  blah: ({ request, route }) => {
    some_shared_logic(request, route.id);
  }
};

[Rich-Harris]

This autocompletion is kinda neat:

Screen.Recording.2022-11-01.at.3.36.06.PM.mov

Also, this typechecking 😍

[Conduitry]

Does that mean this also closes #6167?

[dummdidumm]

no, because that relates to $page.routeId, but you are probably right that this can also be infered from that.

[Rich-Harris]

Ok that was a bit of a faff but it will now correctly identify the dependency on route.id if you have this...

/** @type {import('./$types').LayoutServerLoad} */
export function load({ route }) {
  return { route };
}

.,..and the same goes for url and params. Surprised we didn't get bitten by that bug sooner