Fixes SPA mode error handling

[PH4NTOMiki]

Related to #3969
Needed await.

[changeset-bot]

🦋 Changeset detected

Latest commit: eb414d5

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

[Rich-Harris]

Thanks — can we add a test for this? Not sure how to reproduce the original bug

[PH4NTOMiki]

@Rich-Harris I'm not sure how to test it, I think it would be really flaky. I think the racing occurs with ticks, and init_listeners runs at the same time(before really) as goto and sets initialized to true before the goto checks for it, and then it reloads and again and again...
I could send you the repo or website URL so you can see for yourself.

[PH4NTOMiki]

Also, we are awaiting for hydrate already, so it's not precedent to wait(but I understand that the hydrate function executes faster because it doesn't really have to do network calls), and
the second solution(if we don't want to prolong setting event listeners) is to do something like this:

if (hydrate) await renderer.start(hydrate);
if (router) {
	if (spa) {router.goto(location.href, { replaceState: true }, []).then(()=>{router.initialized = true;});}
	router.init_listeners();
	if (!spa) {router.initialized = true;}
}

and remove this.initialized = true; from inside the router.init_listeners method

and the last option is to use another parameter for goto function and pass it upstream, but I've seen inside #3969 that you discussed it, and decided against it.

So the second solution looks alright.

[PH4NTOMiki]

Actually the second solution could be simplified to this:

if (hydrate) await renderer.start(hydrate);
if (router) {
	router.init_listeners();
	if (spa) await router.goto(location.href, { replaceState: true }, []);
	router.initialized = true;
}

[Rich-Harris]

I just can't reproduce this at all. As far as I can tell, the relevant question is whether the initial NavigationInfo has initial: true or not, which as you know is the inverse of initialized. But even though goto is an async function, that NavigationInfo object is created synchrously — nothing async happens until later.

So I don't understand how it's possible for init_listeners to change initialized in time for info.initial to be incorrect. Without a repro and (ideally) a failing test, it would be irresponsible to merge this, as it makes me think that there's something else going on here.

[PH4NTOMiki]

@Rich-Harris I fully understand that it would be irresponsible to merge it without all the details, so here is a repro
repo https://github.com/PH4NTOMiki/sveltekit-book/tree/ce4d91e76b9ab3fff04fc19a63169eb51f15c670
live link https://0a45804c.sveltekit-book.pages.dev/

.env

VITE_SUPABASE_URL=https://livamxmbpjeypfvlwxyp.supabase.co
VITE_SUPABASE_PUBLIC_KEY=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJyb2xlIjoiYW5vbiIsImlhdCI6MTYzODAzNDYwMSwiZXhwIjoxOTUzNjEwNjAxfQ.WBcJlmIMR3UrrL5zzStTDEPKgi2jYOcO1JbphEF_V20

run npm run dev
and try to load any route other than /, /books, /users and you'll see infinite reloads.

To be honest, when I tried to recreate issue in new repro, I really couldn't easily do it, and as far as I can see supabase import has something to do with it, but the browser network monitor shows that code (if (!navigation_result) {location.href = info.url.href;return;}) as the network call(reload) initiator.

[mrkishi]

So it seems to happen post-initialization on this.update() from the session store subscription:

kit/packages/kit/src/runtime/client/renderer.js

Lines 171 to 179 in 266a460

	this.stores.session.subscribe(async (value) => {
	this.$session = value;
	if (!ready || !this.router) return;
	this.session_id += 1;
	const info = this.router.parse(new URL(location.href));
	if (info) this.update(info, [], true);
	});

[Rich-Harris]

Ah, interesting. That suggests this is the wrong fix — it only appears to fix the issue because the session update happens during the initial hydration. Not sure what the correct fix looks like

[PH4NTOMiki]

@Rich-Harris @mrkishi closing this PR, will try to come up with something more in-depth and send PR.

[PH4NTOMiki]

One possible solution is that we just add another parameter to update function (something similar to that initial parameter) so that we know inside the function that it wasn't run by the click handler, so we can show 404, instead or doing browser navigation to that link. Or am I missing something?