From 48d2aaeeb5fa13439b544b1c64fc49c4c93b727a Mon Sep 17 00:00:00 2001 From: Elia <100457417+Elia872@users.noreply.github.com> Date: Mon, 6 Jan 2025 11:41:58 +0100 Subject: [PATCH] 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 | 5 +++ packages/kit/src/runtime/client/client.js | 39 +++++++++++-------- .../+layout.svelte | 10 +++++ .../a/+page.svelte | 20 ++++++++++ .../b/+page.svelte | 7 ++++ .../basics/test/cross-platform/client.test.js | 8 ++++ 6 files changed, 72 insertions(+), 17 deletions(-) create mode 100644 .changeset/sixty-rabbits-smell.md create mode 100644 packages/kit/test/apps/basics/src/routes/navigation-lifecycle/after-navigate-properly-removed/+layout.svelte create mode 100644 packages/kit/test/apps/basics/src/routes/navigation-lifecycle/after-navigate-properly-removed/a/+page.svelte create mode 100644 packages/kit/test/apps/basics/src/routes/navigation-lifecycle/after-navigate-properly-removed/b/+page.svelte diff --git a/.changeset/sixty-rabbits-smell.md b/.changeset/sixty-rabbits-smell.md new file mode 100644 index 000000000000..c7f66919c926 --- /dev/null +++ b/.changeset/sixty-rabbits-smell.md @@ -0,0 +1,5 @@ +--- +'@sveltejs/kit': patch +--- + +fix: correctly remove navigation callbacks when returning function in onNavigate diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index d0d4038e7ea9..365d7fea3db5 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -191,14 +191,18 @@ const components = []; /** @type {{id: string, token: {}, promise: Promise} | 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,7 +1467,7 @@ 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)) ) ) @@ -1471,14 +1475,16 @@ async function navigate({ 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} 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); }; }); } diff --git a/packages/kit/test/apps/basics/src/routes/navigation-lifecycle/after-navigate-properly-removed/+layout.svelte b/packages/kit/test/apps/basics/src/routes/navigation-lifecycle/after-navigate-properly-removed/+layout.svelte new file mode 100644 index 000000000000..948f89135625 --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/navigation-lifecycle/after-navigate-properly-removed/+layout.svelte @@ -0,0 +1,10 @@ + + +{@render children()} diff --git a/packages/kit/test/apps/basics/src/routes/navigation-lifecycle/after-navigate-properly-removed/a/+page.svelte b/packages/kit/test/apps/basics/src/routes/navigation-lifecycle/after-navigate-properly-removed/a/+page.svelte new file mode 100644 index 000000000000..ddc82eb1ccfd --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/navigation-lifecycle/after-navigate-properly-removed/a/+page.svelte @@ -0,0 +1,20 @@ + + +

/A

+/a +/b diff --git a/packages/kit/test/apps/basics/src/routes/navigation-lifecycle/after-navigate-properly-removed/b/+page.svelte b/packages/kit/test/apps/basics/src/routes/navigation-lifecycle/after-navigate-properly-removed/b/+page.svelte new file mode 100644 index 000000000000..c7a682b6a763 --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/navigation-lifecycle/after-navigate-properly-removed/b/+page.svelte @@ -0,0 +1,7 @@ +

/B

+
+ was called: + false +
+/a +/b diff --git a/packages/kit/test/apps/basics/test/cross-platform/client.test.js b/packages/kit/test/apps/basics/test/cross-platform/client.test.js index c6b7f98cce45..482932514553 100644 --- a/packages/kit/test/apps/basics/test/cross-platform/client.test.js +++ b/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', () => {