fix: correctly remove navigation callbacks when returning function in…

… onNavigate (#13241)

Do that by switching to a Set and keeping a stable reference across call sites.

fixes #13240

.changeset/sixty-rabbits-smell.md

@@ -0,0 +1,5 @@
+---
+'@sveltejs/kit': patch
+---
+
+fix: correctly remove navigation callbacks when returning function in onNavigate

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

@@ -191,14 +191,18 @@ const components = [];
 /** @type {{id: string, token: {}, promise: Promise<import('./types.js').NavigationResult>} | null} */
 let load_cache = null;
 
-/** @type {Array<(navigation: import('@sveltejs/kit').BeforeNavigate) => void>} */
-const before_navigate_callbacks = [];
+/**
+ * Note on before_navigate_callbacks, on_navigate_callbacks and after_navigate_callbacks:
+ * do not re-assign as some closures keep references to these Sets
+ */
+/** @type {Set<(navigation: import('@sveltejs/kit').BeforeNavigate) => void>} */
+const before_navigate_callbacks = new Set();
 
-/** @type {Array<(navigation: import('@sveltejs/kit').OnNavigate) => import('types').MaybePromise<(() => void) | void>>} */
-const on_navigate_callbacks = [];
+/** @type {Set<(navigation: import('@sveltejs/kit').OnNavigate) => import('types').MaybePromise<(() => void) | void>>} */
+const on_navigate_callbacks = new Set();
 
-/** @type {Array<(navigation: import('@sveltejs/kit').AfterNavigate) => void>} */
-let after_navigate_callbacks = [];
+/** @type {Set<(navigation: import('@sveltejs/kit').AfterNavigate) => void>} */
+const after_navigate_callbacks = new Set();
 
 /** @type {import('./types.js').NavigationState} */
 let current = {
@@ -1463,22 +1467,24 @@ async function navigate({
 
 		const after_navigate = (
 			await Promise.all(
-				on_navigate_callbacks.map((fn) =>
+				Array.from(on_navigate_callbacks, (fn) =>
 					fn(/** @type {import('@sveltejs/kit').OnNavigate} */ (nav.navigation))
 				)
 			)
 		).filter(/** @returns {value is () => void} */ (value) => typeof value === 'function');
 
 		if (after_navigate.length > 0) {
 			function cleanup() {
-				after_navigate_callbacks = after_navigate_callbacks.filter(
-					// @ts-ignore
-					(fn) => !after_navigate.includes(fn)
-				);
+				after_navigate.forEach((fn) => {
+					after_navigate_callbacks.delete(fn);
+				});
 			}
 
 			after_navigate.push(cleanup);
-			after_navigate_callbacks.push(...after_navigate);
+
+			after_navigate.forEach((fn) => {
+				after_navigate_callbacks.add(fn);
+			});
 		}
 
 		root.$set(navigation_result.props);
@@ -1680,7 +1686,7 @@ function setup_preload() {
 		}
 	}
 
-	after_navigate_callbacks.push(after_navigate);
+	after_navigate_callbacks.add(after_navigate);
 	after_navigate();
 }
 
@@ -1709,16 +1715,15 @@ function handle_error(error, event) {
 
 /**
  * @template {Function} T
- * @param {T[]} callbacks
+ * @param {Set<T>} callbacks
  * @param {T} callback
  */
 function add_navigation_callback(callbacks, callback) {
 	onMount(() => {
-		callbacks.push(callback);
+		callbacks.add(callback);
 
 		return () => {
-			const i = callbacks.indexOf(callback);
-			callbacks.splice(i, 1);
+			callbacks.delete(callback);
 		};
 	});
 }

packages/kit/test/apps/basics/src/routes/navigation-lifecycle/after-navigate-properly-removed/+layout.svelte

@@ -0,0 +1,10 @@
+<script>
+	import { onNavigate } from '$app/navigation';
+	let { children } = $props();
+
+	onNavigate(() => {
+		return () => {};
+	});
+</script>
+
+{@render children()}

packages/kit/test/apps/basics/src/routes/navigation-lifecycle/after-navigate-properly-removed/a/+page.svelte

@@ -0,0 +1,20 @@
+<script>
+	import { afterNavigate } from '$app/navigation';
+
+	afterNavigate(() => {
+		console.log('after navigate called');
+
+		/**
+		 * @type {HTMLElement}
+		 */
+		const el = document.querySelector('.nav-lifecycle-after-nav-removed-test-target');
+
+		if (el) {
+			el.innerText = 'true';
+		}
+	});
+</script>
+
+<h1>/A</h1>
+<a href="/navigation-lifecycle/after-navigate-properly-removed/a">/a</a>
+<a href="/navigation-lifecycle/after-navigate-properly-removed/b">/b</a>

packages/kit/test/apps/basics/src/routes/navigation-lifecycle/after-navigate-properly-removed/b/+page.svelte

@@ -0,0 +1,7 @@
+<h1>/B</h1>
+<div>
+	was called:
+	<span class="nav-lifecycle-after-nav-removed-test-target">false</span>
+</div>
+<a href="/navigation-lifecycle/after-navigate-properly-removed/a">/a</a>
+<a href="/navigation-lifecycle/after-navigate-properly-removed/b">/b</a>

packages/kit/test/apps/basics/test/cross-platform/client.test.js

@@ -256,6 +256,14 @@ test.describe('Navigation lifecycle functions', () => {
 			'/navigation-lifecycle/on-navigate/a -> /navigation-lifecycle/on-navigate/b (link) true'
 		);
 	});
+
+	test('afterNavigate properly removed', async ({ page, clicknav }) => {
+		await page.goto('/navigation-lifecycle/after-navigate-properly-removed/b');
+		await clicknav('[href="/navigation-lifecycle/after-navigate-properly-removed/a"]');
+		await clicknav('[href="/navigation-lifecycle/after-navigate-properly-removed/b"]');
+
+		expect(await page.textContent('.nav-lifecycle-after-nav-removed-test-target')).toBe('false');
+	});
 });
 
 test.describe('Scrolling', () => {