Defer pushState until navigation succeeds (#4070)

* defer pushState until navigation completes

* fix test, simplify clicknav implementation

* remove now-obsolete sveltekit:navigation-{start,end} events

* changesets

* Update .changeset/little-geckos-smell.md

Co-authored-by: Ignatius Bagus <[email protected]>

* Update documentation/docs/08-events.md

Co-authored-by: Ignatius Bagus <[email protected]>

* make navigation abortable

* sigh. lint

* ugh come on

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

Co-authored-by: Maurício Kishi <[email protected]>

* belt and braces

* ugh

* changeset

* for the love of

* use pushState for redirects

Co-authored-by: Ignatius Bagus <[email protected]>
Co-authored-by: Maurício Kishi <[email protected]>

.changeset/cyan-cobras-explode.md

@@ -0,0 +1,5 @@
+---
+'@sveltejs/kit': patch
+---
+
+[breaking] referer header sent by fetch in load matches page's referer header, not the page itself

.changeset/little-geckos-smell.md

@@ -0,0 +1,5 @@
+---
+'@sveltejs/kit': patch
+---
+
+[breaking] remove sveltekit:navigation-{start,end} events

.changeset/old-years-march.md

@@ -0,0 +1,5 @@
+---
+'@sveltejs/kit': patch
+---
+
+[breaking] defer pushState until navigation occurs

documentation/docs/08-events.md

@@ -2,10 +2,6 @@
 title: Events
 ---
 
-SvelteKit emits [CustomEvents](https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent) on the `window` object when certain things happen:
+SvelteKit emits a `sveltekit:start` [CustomEvent](https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent) on the `window` object once the app has hydrated.
 
-- `sveltekit:start` — fired once the app has hydrated
-- `sveltekit:navigation-start` — navigation has started
-- `sveltekit:navigation-end` — navigation has ended
-
-You probably won't need to use these, but they can be useful in the context of (for example) integration tests.
+You probably won't need to use it, but it can be useful in the context of (for example) integration tests.

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

@@ -335,11 +335,10 @@ export class Renderer {
 				});
 			} else {
 				if (this.router) {
-					this.router.goto(
-						new URL(navigation_result.redirect, info.url).href,
-						{ replaceState: true },
-						[...chain, info.url.pathname]
-					);
+					this.router.goto(new URL(navigation_result.redirect, info.url).href, {}, [
+						...chain,
+						info.url.pathname
+					]);
 				} else {
 					location.href = new URL(navigation_result.redirect, location.href).href;
 				}

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

@@ -425,32 +425,32 @@ export class Router {
 
 		accepted();
 
-		if (!this.navigating) {
-			dispatchEvent(new CustomEvent('sveltekit:navigation-start'));
-		}
 		this.navigating++;
 
 		const pathname = normalize_path(url.pathname, this.trailing_slash);
 
 		info.url = new URL(url.origin + pathname + url.search + url.hash);
 
-		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);
-		}
+		const token = (this.navigating_token = {});
 
 		await this.renderer.handle_navigation(info, chain, false, {
 			scroll,
 			keepfocus
 		});
 
 		this.navigating--;
-		if (!this.navigating) {
-			dispatchEvent(new CustomEvent('sveltekit:navigation-end'));
 
+		// navigation was aborted
+		if (this.navigating_token !== token) return;
+		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);
+		}
 	}
 }

packages/kit/src/runtime/server/page/load_node.js

@@ -139,8 +139,6 @@ export async function load_node({
 					}
 				}
 
-				opts.headers.set('referer', event.url.href);
-
 				const resolved = resolve(event.url.pathname, requested.split('?')[0]);
 
 				/** @type {Response} */

packages/kit/test/ambient.d.ts

@@ -6,6 +6,7 @@ declare global {
 		// used in tests
 		oops: string;
 		pageContext: any;
+		fulfil_navigation: (value: any) => void;
 	}
 
 	const goto: (

packages/kit/test/apps/basics/src/routes/routing/cancellation/__layout.svelte

@@ -0,0 +1,4 @@
+<slot></slot>
+
+<a href="/routing/cancellation/a">a</a>
+<a href="/routing/cancellation/b">b</a>

packages/kit/test/apps/basics/src/routes/routing/cancellation/a.svelte

@@ -0,0 +1,17 @@
+<script context="module">
+import { browser } from '$app/env';
+
+
+	/** @type {import('@sveltejs/kit').Load} */
+	export async function load() {
+		if (browser) {
+			await new Promise(f => {
+				window.fulfil_navigation = f;
+			});
+		}
+
+		return {};
+	}
+</script>
+
+<h1>this should not appear</h1>

packages/kit/test/apps/basics/src/routes/routing/cancellation/b.svelte

@@ -0,0 +1 @@
+<h1>b</h1>

packages/kit/test/apps/basics/src/routes/routing/cancellation/index.svelte

@@ -0,0 +1 @@
+<h1>cancellation</h1>

packages/kit/test/apps/basics/test/test.js

@@ -106,10 +106,10 @@ test.describe.parallel('beforeNavigate', () => {
 		await page.goto('/before-navigate/prevent-navigation');
 
 		try {
-			await clicknav('[href="/before-navigate/a"]');
+			await clicknav('[href="/before-navigate/a"]', { timeout: 1000 });
 			expect(false).toBe(true);
 		} catch (/** @type {any} */ e) {
-			expect(e.message).toMatch('Timed out');
+			expect(e.message).toMatch('page.waitForNavigation: Timeout 1000ms exceeded');
 		}
 
 		expect(page.url()).toBe(baseURL + '/before-navigate/prevent-navigation');
@@ -1269,8 +1269,12 @@ test.describe.parallel('Load', () => {
 		await clicknav('[href="/load/fetch-headers"]');
 
 		const json = /** @type {string} */ (await page.textContent('pre'));
-		expect(JSON.parse(json)).toEqual({
-			referer: `${baseURL}/load/fetch-headers`,
+		const headers = JSON.parse(json);
+
+		expect(headers).toEqual({
+			// the referer will be the previous page in the client-side
+			// navigation case
+			referer: `${baseURL}/load`,
 			// these headers aren't particularly useful, but they allow us to verify
 			// that page headers are being forwarded
 			'sec-fetch-dest': javaScriptEnabled ? 'empty' : 'document',
@@ -1631,13 +1635,17 @@ test.describe.parallel('searchParams', () => {
 });
 
 test.describe.parallel('Redirects', () => {
-	test('redirect', async ({ page, clicknav }) => {
+	test('redirect', async ({ baseURL, page, clicknav, back }) => {
 		await page.goto('/redirect');
 
 		await clicknav('[href="/redirect/a"]');
 
 		await page.waitForURL('/redirect/c');
 		expect(await page.textContent('h1')).toBe('c');
+		expect(page.url()).toBe(`${baseURL}/redirect/c`);
+
+		await back();
+		expect(page.url()).toBe(`${baseURL}/redirect`);
 	});
 
 	test('prevents redirect loops', async ({ baseURL, page, javaScriptEnabled }) => {
@@ -2123,6 +2131,21 @@ test.describe.parallel('Routing', () => {
 		await clicknav('[href="/static.json"]');
 		expect(await page.textContent('body')).toBe('"static file"\n');
 	});
+
+	test('navigation is cancelled upon subsequent navigation', async ({
+		baseURL,
+		page,
+		clicknav
+	}) => {
+		await page.goto('/routing/cancellation');
+		await page.click('[href="/routing/cancellation/a"]');
+		await clicknav('[href="/routing/cancellation/b"]');
+
+		expect(await page.url()).toBe(`${baseURL}/routing/cancellation/b`);
+
+		await page.evaluate('window.fulfil_navigation && window.fulfil_navigation()');
+		expect(await page.url()).toBe(`${baseURL}/routing/cancellation/b`);
+	});
 });
 
 test.describe.parallel('Session', () => {

packages/kit/test/utils.d.ts

@@ -20,7 +20,7 @@ export const test: TestType<
 				prefetchRoutes: (urls: string[]) => Promise<void>;
 			};
 			back: () => Promise<void>;
-			clicknav: (selector: string) => Promise<void>;
+			clicknav: (selector: string, options?: { timeout?: number }) => Promise<void>;
 			in_view: (selector: string) => Promise<boolean>;
 			read_errors: (href: string) => string;
 		},

packages/kit/test/utils.js

@@ -58,20 +58,6 @@ export const test = base.extend({
 	back: async ({ page, javaScriptEnabled }, use) => {
 		use(async () => {
 			if (javaScriptEnabled) {
-				await page.evaluate(() => {
-					window.navigated = new Promise((fulfil, reject) => {
-						const timeout = setTimeout(() => reject(new Error('Timed out')), 2000);
-						addEventListener(
-							'sveltekit:navigation-end',
-							() => {
-								clearTimeout(timeout);
-								fulfil();
-							},
-							{ once: true }
-						);
-					});
-				});
-
 				await Promise.all([page.goBack(), page.evaluate(() => window.navigated)]);
 			} else {
 				return page.goBack().then(() => void 0);
@@ -81,24 +67,13 @@ export const test = base.extend({
 
 	// @ts-expect-error
 	clicknav: async ({ page, javaScriptEnabled }, use) => {
-		/** @param {string} selector */
-		async function clicknav(selector) {
+		/**
+		 * @param {string} selector
+		 * @param {{ timeout: number }} options
+		 */
+		async function clicknav(selector, options) {
 			if (javaScriptEnabled) {
-				await page.evaluate(() => {
-					window.navigated = new Promise((fulfil, reject) => {
-						const timeout = setTimeout(() => reject(new Error('Timed out')), 2000);
-						addEventListener(
-							'sveltekit:navigation-end',
-							() => {
-								clearTimeout(timeout);
-								fulfil();
-							},
-							{ once: true }
-						);
-					});
-				});
-
-				await Promise.all([page.click(selector), page.evaluate(() => window.navigated)]);
+				await Promise.all([page.click(selector), page.waitForNavigation(options)]);
 			} else {
 				await page.click(selector);
 			}