From 2db01fb33fb4789aa0ddd75787432ca3450d36f7 Mon Sep 17 00:00:00 2001
From: Ben McCann <322311+benmccann@users.noreply.github.com>
Date: Wed, 20 Oct 2021 13:34:05 -0700
Subject: [PATCH] [fix] fire navigation-end event only at end of navigation
(#2649)
---
.changeset/chatty-apricots-fly.md | 5 +++
packages/kit/src/runtime/client/renderer.js | 20 +++++----
packages/kit/src/runtime/client/router.js | 17 +++++---
.../apps/basics/src/routes/redirect/_tests.js | 2 +-
.../apps/basics/src/routes/redirect/c.svelte | 2 +-
.../apps/basics/src/routes/routing/_tests.js | 12 ++----
.../src/routes/routing/shadow/_tests.js | 4 +-
packages/kit/test/test.js | 42 ++++++++++++-------
8 files changed, 64 insertions(+), 40 deletions(-)
create mode 100644 .changeset/chatty-apricots-fly.md
diff --git a/.changeset/chatty-apricots-fly.md b/.changeset/chatty-apricots-fly.md
new file mode 100644
index 000000000000..fed096789767
--- /dev/null
+++ 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
index 92e9f1cf5d4d..79d15f9600d5 100644
--- a/packages/kit/src/runtime/client/renderer.js
+++ b/packages/kit/src/runtime/client/renderer.js
@@ -204,10 +204,13 @@ 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: {
@@ -215,11 +218,13 @@ export class Renderer {
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
index 9c9a01023982..79c8cb7f710c 100644
--- 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
index 9522daabe147..2c76ba30b1a1 100644
--- 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
index 70f3d5c81534..232c276f86cb 100644
--- a/packages/kit/test/apps/basics/src/routes/redirect/c.svelte
+++ b/packages/kit/test/apps/basics/src/routes/redirect/c.svelte
@@ -1 +1 @@
-c
\ No newline at end of file
+c
diff --git a/packages/kit/test/apps/basics/src/routes/routing/_tests.js b/packages/kit/test/apps/basics/src/routes/routing/_tests.js
index e2a138a40af1..6a3a8033b488 100644
--- 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
index 0cf22616fdf9..a11629b300d0 100644
--- 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
index 2549c4023020..1c89179c412b 100644
--- 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