fix: ensure search params are not left off of load_cache

[iteratetograceness]

Closes #10122.

The load_cache has no context of any search parameters that exist on the current path. So when data-sveltekit-preload-data is enabled for links, search parameters were being left off in the navigate function. More specifically, when setting the current variable which is used to inform the from property in BeforeNavigate.

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

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

[ghostdevv]

LGTM, hopefully another maintainer can take a look soon to give feedback 🙏

[iteratetograceness]

I will go bug @tcc-sejohnson for a review!

[elliott-with-the-longest-name-on-github]

Hmmm, I'm not sure if this is quite right... if I'm understanding correctly, preloading /foo?bar=baz and then clicking a link for /foo would still navigate to /foo?bar=baz. Maybe I'm wrong -- I'm reading code on mobile. I'll dig in further when I get home.

[iteratetograceness]

@tcc-sejohnson this was actually a bit confusing at first for me too!

I'll try my best at a better description:

We're not preloading links that have a search param but without (/foo); when we navigate to /foo, what's saved in the load_cache is /foo. When we have logic inside a beforeNavigate to persist a search param using to and from, the search param ends up getting lost after the first navigation because the value for to gets set via the current variable -- and current gets set using the load_cache (which in this example currently holds /foo not something like /foo?bar=baz).

Please do let me know if you see a better way to tackle this! For a better understanding of the bug, it'll help to start from the repo in the issue linked or the added test.

[dummdidumm]

Thanks for tackling this!

I think the fix is applied at a point that is too late. If the the search param is appended, a load function dealing with it should see that search param, too - which is not the case with this fix. So I think instead the load_cache should be ignored completely if a mismatch is detected. The root problem is that the id which determines this is computed in get_navigation_intent which is before calling beforeNavigate which could change the search params and therefore the id.

In general I'm wondering if we actually want to fix this. what would happen if someone changes the pathname in beforeNavigate? Then we're at a completely different page. Should we handle that? Should we instead document that these parameters should be treated as readonly and tell people to instead do something like this?

beforeNavigate(({ from, to, cancel }) => {
	const previous_param_value = from?.url.searchParams.get(preserved_param);
	if (typeof previous_param_value === 'string') {
		const url = new URL(to.url);
		url.searchParams.set(preserved_param, 'asd');
		cancel();
		goto(url);
	}
});

Then again, beforeNavigate has no info about any of the additional goto parameters (like noScroll), and it would also need additional checks to handle goto/invalidate etc correctly. So I could also be persuaded to say "search params are a special case which we handle". I'm not sure what the correct approach here is yet.

[iteratetograceness]

@dummdidumm really appreciate you taking a look!

Based on what you've shared, I'm also wondering whether this is something we want to explicitly fix -- and whether there's an appropriate "fix" for this based on how/the order of navigation and preloading are currently handled.

I can take some more time this week/next to dig a little deeper on the flow of navigation and see if there may be a better approach!

outdated

[Rich-Harris]

preview: https://svelte-dev-git-preview-kit-10179-svelte.vercel.app/

this is an automated message

[eltigerchino]

Closing this since I can't reproduce this original issue after upgrading to the latest version of SvelteKit.