fix: prevent navigation when preloading fails due to network error

[eltigerchino]

fixes #9508

Adds a conditional check to differentiate between a preload (data-sveltekit-preload-data) running load_route and a load_route from actual navigation. We only want to load the root error page for actual navigations and not preloads.

---

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.

[changeset-bot]

🦋 Changeset detected

Latest commit: f96b431

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]

Thanks for tackling this!
I believe the same can happen if you hover a link, then the module doesn't load, then SvelteKit checks for a new version, sees that there is one, and does a hard navigation - we also don't want that if we're in preload mode.

I'm also not sure if preload as a parameter works, because someone could hover, which starts preloading, then click, which means it's no longer in preload mode, but since the boolean is tied to the request and not a global flag, the "is in preload mode" logic will kick in wrongfully.
Not sure how to best solve this.

[dummdidumm]

What does the return statement do? I'm wondering what happens if there's an error, and then the user actually clicks on the link? The return statement looks a bit like a placeholder to me.

[eltigerchino]

I believe it prevents the load data from being cached. I was trying to match the shape to make the conditional below succeed.

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

Lines 380 to 386 in 6107466

	promise: load_route({ ...intent, preload: true }).then((result) => {
	if (result.type === 'loaded' && result.state.error) {
	// Don't cache errors, because they might be transient
	load_cache = null;
	}
	return result;
	})

If the user clicks on the link it would try loading again (and trigger the error page if the network is still offline). EDIT: I think I see now. It doesn't work for ongoing network requests since the cache hasn't been emptied yet.

[dummdidumm]

Maybe something similar like we do today already with the navigation token:

• set a preload token (empty object, unique key basically)
• when the user actually clicks on a link, the preload token becomes the navigation token
• when an error occurs, the preload token passed to the navigation is compared with the current navigation token, and if they're the same we know that we can proceed with the logic as-is (i.e. the preload became the real navigation). Else we return a fake error state, or we throw and the preload catches that

[eltigerchino]

Maybe something similar like we do today already with the navigation token:

• set a preload token (empty object, unique key basically)
• when the user actually clicks on a link, the preload token becomes the navigation token
• when an error occurs, the preload token passed to the navigation is compared with the current navigation token, and if they're the same we know that we can proceed with the logic as-is (i.e. the preload became the real navigation). Else we return a fake error state, or we throw and the preload catches that

I tried to implement the preload token idea. I hope I understood the idea correctly because it seems pretty brilliant (everything seems to just work now?).

EDIT: there might still be some cleanup to do to make things DRY-er but I haven't figured out how yet (going to bed now).

[dummdidumm]

This isn't quite right yet I think. Imagine you navigate to a route, but that's slow, and for some reason there's a preload happening during that time, then the real navigation would be aborted because the preload did update the token. I think we need another global next to token, preload_token. It is set inside _preload_data and (like now) passed to load_route and if the real navigation is a different one to the preloaded one, it resets the preload_token so that the if condition in load_route doesn't kick in.

[dummdidumm]

This already happens in _preload_route, I think we don't need it here. Also, we can probably create a function that encapsulates the return statement for reuse (minifies a bit better). More generally it makes me wonder if there should be another type for preload_data specifically, like type: 'preload_error' - we can't do that until 3.0 though.

[eltigerchino]

Thanks for the guidance! I hope I managed to correctly understand and implement it this time. I also added a test for the "slow navigation into preload" case.

[dummdidumm]

Gave this another look and I realized that there were still edge cases where things could break when multiple preloads where being executed simulatenously. I concluded that we instead need a set of tokens, and add/remove to/from it when the preload starts/ends/becomes the real navigation. I adjusted the code - let me know what you think.

[eltigerchino]

Looks good! Shall I try and add a test for the edge case?

[dummdidumm]

I think we're good with the tests 👍