[fix] fire navigation-end event only at end of navigation (#2649)

sveltejs · Oct 20, 2021 · 2db01fb · 2db01fb
1 parent 8df532b
commit 2db01fb
Show file tree

Hide file tree

Showing 8 changed files with 64 additions and 40 deletions.
diff --git a/.changeset/chatty-apricots-fly.md b/.changeset/chatty-apricots-fly.md
@@ -0,0 +1,5 @@
+---
+'@sveltejs/kit': patch
+---
+
+[fix] fire navigation-end event only at end of navigation
diff --git a/packages/kit/src/runtime/client/renderer.js b/packages/kit/src/runtime/client/renderer.js
@@ -204,22 +204,27 @@ export class Renderer {
 		this._init(result);
 	}
 
-	/** @param {{ path: string, query: URLSearchParams }} destination */
-	notify({ path, query }) {
-		dispatchEvent(new CustomEvent('sveltekit:navigation-start'));
-
+	/**
+	 * @param {import('./types').NavigationInfo} info
+	 * @param {string[]} chain
+	 * @param {boolean} no_cache
+	 * @param {{hash?: string, scroll: { x: number, y: number } | null, keepfocus: boolean}} [opts]
+	 */
+	async handle_navigation(info, chain, no_cache, opts) {
 		if (this.started) {
 			this.stores.navigating.set({
 				from: {
 					path: this.current.page.path,
 					query: this.current.page.query
 				},
 				to: {
-					path,
-					query
+					path: info.path,
+					query: info.query
 				}
 			});
 		}
+
+		await this.update(info, chain, no_cache, opts);
 	}
 
 	/**
@@ -291,11 +296,12 @@ export class Renderer {
 		}
 
 		await 0;
-		dispatchEvent(new CustomEvent('sveltekit:navigation-end'));
+
 		this.loading.promise = null;
 		this.loading.id = null;
 
 		if (!this.router) return;
+
 		const leaf_node = navigation_result.state.branch[navigation_result.state.branch.length - 1];
 		if (leaf_node && leaf_node.module.router === false) {
 			this.router.disable();

diff --git a/packages/kit/src/runtime/client/router.js b/packages/kit/src/runtime/client/router.js
@@ -39,6 +39,8 @@ export class Router {
 		this.base = base;
 		this.routes = routes;
 		this.trailing_slash = trailing_slash;
+		/** Keeps tracks of multiple navigations caused by redirects during rendering */
+		this.navigating = 0;
 
 		/** @type {import('./renderer').Renderer} */
 		this.renderer = renderer;
@@ -253,6 +255,11 @@ export class Router {
 			throw new Error('Attempted to navigate to a URL that does not belong to this app');
 		}
 
+		if (!this.navigating) {
+			dispatchEvent(new CustomEvent('sveltekit:navigation-start'));
+		}
+		this.navigating++;
+
 		// remove trailing slashes
 		if (info.path !== '/') {
 			const has_trailing_slash = info.path.endsWith('/');
@@ -269,11 +276,11 @@ export class Router {
 			}
 		}
 
-		this.renderer.notify({
-			path: info.path,
-			query: info.query
-		});
+		await this.renderer.handle_navigation(info, chain, false, { hash, scroll, keepfocus });
 
-		await this.renderer.update(info, chain, false, { hash, scroll, keepfocus });
+		this.navigating--;
+		if (!this.navigating) {
+			dispatchEvent(new CustomEvent('sveltekit:navigation-end'));
+		}
 	}
 }
diff --git a/packages/kit/test/apps/basics/src/routes/redirect/_tests.js b/packages/kit/test/apps/basics/src/routes/redirect/_tests.js
@@ -5,7 +5,7 @@ export default function (test) {
 	test('redirect', '/redirect', async ({ base, page, clicknav }) => {
 		await clicknav('[href="/redirect/a"]');
 
-		assert.equal(page.url(), `${base}/redirect/c`);
+		await page.waitForURL(`${base}/redirect/c`);
 		assert.equal(await page.textContent('h1'), 'c');
 	});
 

diff --git a/packages/kit/test/apps/basics/src/routes/redirect/c.svelte b/packages/kit/test/apps/basics/src/routes/redirect/c.svelte
@@ -1 +1 @@
-<h1>c</h1>
+<h1>c</h1>
diff --git a/packages/kit/test/apps/basics/src/routes/routing/_tests.js b/packages/kit/test/apps/basics/src/routes/routing/_tests.js
@@ -233,14 +233,10 @@ export default function (test, is_dev) {
 		}
 	);
 
-	test(
-		'ignores navigation to URLs the app does not own',
-		'/routing',
-		async ({ page, clicknav }) => {
-			await clicknav('[href="https://www.google.com"]');
-			assert.equal(page.url(), 'https://www.google.com/');
-		}
-	);
+	test('ignores navigation to URLs the app does not own', '/routing', async ({ page }) => {
+		await page.click('[href="https://www.google.com"]');
+		assert.equal(page.url(), 'https://www.google.com/');
+	});
 
 	// skipping this test because it causes a bunch of failures locally
 	test.skip('watch new route in dev', '/routing', async ({ page, base, js, watcher }) => {

diff --git a/packages/kit/test/apps/basics/src/routes/routing/shadow/_tests.js b/packages/kit/test/apps/basics/src/routes/routing/shadow/_tests.js
@@ -2,7 +2,7 @@ import * as assert from 'uvu/assert';
 
 /** @type {import('test').TestMaker} */
 export default function (test) {
-	test('endpoints can shadow pages', '/routing/shadow', async ({ page, clicknav }) => {
+	test('endpoints can shadow pages', '/routing/shadow', async ({ page }) => {
 		const random = String(Math.random());
 
 		await page.evaluate((random) => {
@@ -11,7 +11,7 @@ export default function (test) {
 			el.value = random;
 		}, random);
 
-		await clicknav('button');
+		await page.click('button');
 
 		assert.equal(await page.textContent('h1'), random);
 	});

diff --git a/packages/kit/test/test.js b/packages/kit/test/test.js
@@ -217,32 +217,42 @@ function duplicate(test_fn, config, is_build) {
 					clicknav: async (selector) => {
 						await context.pages.js.evaluate(() => {
 							window.navigated = new Promise((fulfil, reject) => {
-								addEventListener('sveltekit:navigation-end', function handler() {
-									fulfil();
-									removeEventListener('sveltekit:navigation-end', handler);
-								});
-
-								setTimeout(() => reject(new Error('Timed out')), 2000);
+								const timeout = setTimeout(() => reject(new Error('Timed out')), 2000);
+								addEventListener(
+									'sveltekit:navigation-end',
+									() => {
+										clearTimeout(timeout);
+										fulfil();
+									},
+									{ once: true }
+								);
 							});
 						});
 
-						await context.pages.js.click(selector);
-						await context.pages.js.evaluate(() => window.navigated);
+						await Promise.all([
+							context.pages.js.click(selector),
+							context.pages.js.evaluate(() => window.navigated)
+						]);
 					},
 					back: async () => {
 						await context.pages.js.evaluate(() => {
 							window.navigated = new Promise((fulfil, reject) => {
-								addEventListener('sveltekit:navigation-end', function handler() {
-									fulfil();
-									removeEventListener('sveltekit:navigation-end', handler);
-								});
-
-								setTimeout(() => reject(new Error('Timed out')), 2000);
+								const timeout = setTimeout(() => reject(new Error('Timed out')), 2000);
+								addEventListener(
+									'sveltekit:navigation-end',
+									() => {
+										clearTimeout(timeout);
+										fulfil();
+									},
+									{ once: true }
+								);
 							});
 						});
 
-						await context.pages.js.goBack();
-						await context.pages.js.evaluate(() => window.navigated);
+						await Promise.all([
+							context.pages.js.goBack(),
+							context.pages.js.evaluate(() => window.navigated)
+						]);
 					},
 					js: true,
 					// @ts-expect-error