Defer pushState until navigation succeeds

[Rich-Harris]

See #2870, and also #4056. This changes the order of operations when navigating: rather than immediately doing a pushState, so that the URL changes as soon as you click a link, it waits for navigation to happen first.

This is arguably less good UX since it means a user may get no immediate feedback that navigation is occurring. But it more closely mirrors native browser behaviour, and fixes a couple of issues:

• Relative URLs can become broken for the interval between the URL updating and navigation taking place, meaning that if a user were to click a relative link while a navigation was pending, they might end up going somewhere nonsensical
• We have to jump through absurd hoops when testing in order to guarantee that navigation has completed. (We still have to jump through some hoops — namely, we need to await page.waitForNavigation() as well as page.click(...), but that is at least a normal thing people have to do with Playwright.)

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 pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

[changeset-bot]

🦋 Changeset detected

Latest commit: e28bc39

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]

One unavoidable side-effect of this change: the referer header for resources in load will be the previous page, not the page the load belongs to. For consistency between server and client, the referer header is no longer overwritten in the server implementation of fetch.

[Rich-Harris]

would be good to do client-side hooks so that we can get rid of this last custom event

[PH4NTOMiki]

• Relative URLs can become broken for the interval between the URL updating and navigation taking place, meaning that if a user were to click a relative link while a navigation was pending, they might end up going somewhere nonsensical

I actually think that we can use page.url store(or some variable) and read from it for navigation purposes(for making relative URLs) inside of click event handler instead of doing this whole change.
But this could be used for other thing.

[Conduitry]

We can only manually resolve relative links in situations where the router handles the navigation. If a link has a target attribute or if the user middle clicks it to open in a new tab, the browser will get the URL wrong.

[PH4NTOMiki]

@Conduitry I think you're somewhat right, we are listening to click event handler for window, so we can do the whole routing(event the "browser" part of it) internally, for ex. for rel=external do this, resolve the link with page store, and then do location.href = resolvedUrl;
for ctrl(should be the same as middle click if I'm not mistaken, I personally never use mouse, only laptop touchpad, so I'm not sure about this, please correct me if I'm wrong) event.preventDefault and window.open(resolvedUrl, '_blank');
This is just brainstorming.

I don't think we have to follow other's rules, but changing this means changing behavior that every other framework defined. Like @Rich-Harris himself wrote in some previous discussion, Youtube, FB and others (and that means user expect it) are using this behavior and now we're changing it.

[Rich-Harris]

It's not just links. If you happen to load an image during that limbo period, or do a fetch, or any one of a bunch of other things that involve URLs, your app will be broken. These are edge cases, but it's not good enough to say 'the framework is broken but you probably won't notice'.

By far the most widely used SSR framework that does client-side navigation is Next, and they also defer pushState, so if we're basing our decision on an appeal to authority then this PR is the right move.

[PH4NTOMiki]

Okay, then this move makes sense.

[PH4NTOMiki]

And I'm not sure about this, so I have to ask, If for ex. person redeploys Kit, and 500 error(missing module) occurs would this PR change URL, so the user can safely reload page?
If that's true, I'm all for this change.

[Rich-Harris]

Yes, the URL will update when the navigation is settled, whether or not it's 'successful'

[mrkishi]

This is a positive change imo—it's something I was wary since Sapper.

I haven't had the time to properly review and test it just yet, but wouldn't this add to/replace history even when a navigation was aborted? Couldn't it also run the after_navigate callbacks with the wrong url if the aborted navigation took longer to complete than newer ones?

Renderer holds a token to the latest navigation in order to abort old ones:

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

Line 325 in 1db3a31

if (token !== this.token) return;

While Router only keeps track of how many are in-flight:

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

Lines 422 to 443 in 1db3a31

	this.navigating++;
	const pathname = normalize_path(url.pathname, this.trailing_slash);
	info.url = new URL(url.origin + pathname + url.search + url.hash);
	await this.renderer.handle_navigation(info, chain, false, {
	scroll,
	keepfocus
	});
	this.navigating--;
	if (!this.navigating) {
	const navigation = { from, to: url };
	this.callbacks.after_navigate.forEach((fn) => fn(navigation));
	}
	if (details) {
	const change = details.replaceState ? 0 : 1;
	details.state['sveltekit:index'] = this.current_history_index += change;
	history[details.replaceState ? 'replaceState' : 'pushState'](details.state, '', info.url);
	}

Granted, if this is actually what's happening, it'd have already been the case prior to this change, only with a slightly different outcome.

[benmccann]

I'm in favor of this change merely because of integration testing. I understand how things worked prior to this PR well enough to get our integration tests working, but I'm not sure our users could be expected to. Even if we provided the utilities and documentation, I still expect it would be an endless source of confusion

[Rich-Harris]

good catch @mrkishi — adding a test and fixing that case

[dummdidumm]

One unavoidable side-effect of this change: the referer header for resources in load will be the previous page, not the page the load belongs to. For consistency between server and client, the referer header is no longer overwritten in the server implementation of fetch.

Sounds like a breaking change that should be mentioned in the PR description along with the other breaking changes for people to find when they check the changelog and check for details in the PR how to migrate.

[Rich-Harris]

good catch, added another changeset

[rmunn]

I'm in favor of this change merely because of integration testing. I understand how things worked prior to this PR well enough to get our integration tests working, but I'm not sure our users could be expected to. Even if we provided the utilities and documentation, I still expect it would be an endless source of confusion

Just to add one more data point in favor of this change, I was recently helping a new developer with some end-to-end tests using Playwright. She was learning it by writing tests against the Playwright website. Their site has the behavior that Svelte used to have: the URL changes immediately, and then the content updates some milliseconds later. And her test, which was looking for the first <h2> on the page, was failing because it was matching the <h2> from the old page rather than the new page. Playwright's auto-wait-for-navigation feature was failing on their own site, because it was changing the URL first. That caused the wait-for-navigation feature to think navigation was completed, so the test went on to the next line which compared the <h2> text content against expected values, and usually failed because of the race condition between loading the new content and the end-to-end test getting to the expect.

That was very confusing for a new dev, so I'm glad that this decision was made. Because now sites using Svelte won't cause that kind of confusion to other new devs.

[gustavopch]

@Rich-Harris In my application, I pass state to goto in many situations, so I read window.history.state in the load function of the new route. Since this PR, window.history.state is only updated after load runs, so when I read it, I'm reading the state of the previous route instead of the one that was just passed to goto.

Maybe the new state should be passed to the load function?

[cheesycod]

<svelte:window
on:sveltekit:navigation-start={() => {
console.log("Set loading")
$inputstore = []
$loadstore = "Loading..."
$navigationState = 'loading';
}}
on:sveltekit:navigation-end={() => {
console.log("Set loaded")
$navigationState = 'loaded';
}}
/>

so this would no longer work?