reset navigation store to null after aborted nav (#4664)

* reset navigation store to null after aborted nav

* remove unused variable

Author: Rich-Harris

.changeset/six-garlics-film.md

@@ -0,0 +1,5 @@
+---
+'@sveltejs/kit': patch
+---
+
+Navigation to current URL is no longer a no-op

.changeset/sour-penguins-admire.md

@@ -0,0 +1,5 @@
+---
+'@sveltejs/kit': patch
+---
+
+navigation store resets to null after aborted nav

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

@@ -121,9 +121,6 @@ export function create_client({ target, session, base, trailing_slash }) {
 	});
 	ready = true;
 
-	/** Keeps tracks of multiple navigations caused by redirects during rendering */
-	let navigating = 0;
-
 	let router_enabled = true;
 
 	// keeping track of the history index in order to prevent popstate navigation events if needed
@@ -158,9 +155,6 @@ export function create_client({ target, session, base, trailing_slash }) {
 	/** @type {{}} */
 	let token;
 
-	/** @type {{}} */
-	let navigating_token;
-
 	/**
 	 * @param {string} href
 	 * @param {{ noscroll?: boolean; replaceState?: boolean; keepfocus?: boolean; state?: any }} opts
@@ -206,6 +200,7 @@ export function create_client({ target, session, base, trailing_slash }) {
 	}
 
 	/**
+	 * Returns `true` if update completes, `false` if it is aborted
 	 * @param {URL} url
 	 * @param {string[]} redirect_chain
 	 * @param {boolean} no_cache
@@ -237,11 +232,11 @@ export function create_client({ target, session, base, trailing_slash }) {
 
 		if (!navigation_result) {
 			await native_navigation(url);
-			return; // unnecessary, but TypeScript prefers it this way
+			return false; // unnecessary, but TypeScript prefers it this way
 		}
 
 		// abort if user navigated during update
-		if (token !== current_token) return;
+		if (token !== current_token) return false;
 
 		invalidated.length = 0;
 
@@ -263,7 +258,7 @@ export function create_client({ target, session, base, trailing_slash }) {
 					await native_navigation(new URL(navigation_result.redirect, location.href));
 				}
 
-				return;
+				return false;
 			}
 		} else if (navigation_result.props?.page?.status >= 400) {
 			const updated = await stores.updated.check();
@@ -346,6 +341,8 @@ export function create_client({ target, session, base, trailing_slash }) {
 
 		const leaf_node = navigation_result.state.branch[navigation_result.state.branch.length - 1];
 		router_enabled = leaf_node?.module.router !== false;
+
+		return true;
 	}
 
 	/** @param {import('./types').NavigationResult} result */
@@ -898,29 +895,20 @@ export function create_client({ target, session, base, trailing_slash }) {
 
 		accepted();
 
-		navigating++;
-
-		const current_navigating_token = (navigating_token = {});
-
 		if (started) {
 			stores.navigating.set({
 				from: current.url,
 				to: normalized
 			});
 		}
 
-		await update(normalized, redirect_chain, false, {
+		const completed = await update(normalized, redirect_chain, false, {
 			scroll,
 			keepfocus,
 			details
 		});
 
-		navigating--;
-
-		// navigation was aborted
-		if (navigating_token !== current_navigating_token) return;
-
-		if (!navigating) {
+		if (completed) {
 			const navigation = { from, to: normalized };
 			callbacks.after_navigate.forEach((fn) => fn(navigation));
 
@@ -1113,11 +1101,6 @@ export function create_client({ target, session, base, trailing_slash }) {
 				// Ignore if <a> has a target
 				if (is_svg_a_element ? a.target.baseVal : a.target) return;
 
-				if (url.href === location.href) {
-					if (!location.hash) event.preventDefault();
-					return;
-				}
-
 				// Check if new url only differs by hash and use the browser default behavior in that case
 				// This will ensure the `hashchange` event is fired
 				// Removing the hash does a full page navigation in the browser, so make sure a hash is present
@@ -1142,7 +1125,7 @@ export function create_client({ target, session, base, trailing_slash }) {
 					redirect_chain: [],
 					details: {
 						state: {},
-						replaceState: false
+						replaceState: url.href === location.href
 					},
 					accepted: () => event.preventDefault(),
 					blocked: () => event.preventDefault()

packages/kit/test/apps/basics/src/routes/store/navigating/__layout.svelte

@@ -2,7 +2,11 @@
 	import { navigating } from '$app/stores';
 </script>
 
-<nav><a href="/store/navigating/a">a</a> <a href="/store/navigating/b">b</a></nav>
+<nav>
+	<a href="/store/navigating/a">a</a>
+	<a href="/store/navigating/b">b</a>
+	<a href="/store/navigating/c">c</a>
+</nav>
 
 <div id="nav-status">
 	{#if $navigating}

packages/kit/test/apps/basics/src/routes/store/navigating/c.svelte

@@ -0,0 +1,9 @@
+<script context="module">
+	/** @type {import('@sveltejs/kit').Load} */
+	export async function load() {
+		await new Promise((f) => setTimeout(f, 1000));
+		return {};
+	}
+</script>
+
+<p>c</p>

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

@@ -1696,6 +1696,21 @@ test.describe.parallel('$app/stores', () => {
 			expect(await page.textContent('#nav-status')).toBe('not currently navigating');
 		}
 	});
+
+	test('navigating store clears after aborted navigation', async ({ page, javaScriptEnabled }) => {
+		await page.goto('/store/navigating/a');
+
+		expect(await page.textContent('#nav-status')).toBe('not currently navigating');
+
+		if (javaScriptEnabled) {
+			page.click('a[href="/store/navigating/c"]');
+			await page.waitForTimeout(100); // gross, but necessary since no navigation occurs
+			page.click('a[href="/store/navigating/a"]');
+
+			await page.waitForSelector('#not-navigating', { timeout: 500 });
+			expect(await page.textContent('#nav-status')).toBe('not currently navigating');
+		}
+	});
 });
 
 test.describe.parallel('searchParams', () => {