From fd8bca9796e2a9dbdbe27ae147393e70fcc6ce77 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Wed, 23 Feb 2022 13:48:01 -0500 Subject: [PATCH] Defer pushState until navigation succeeds (#4070) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * defer pushState until navigation completes * fix test, simplify clicknav implementation * remove now-obsolete sveltekit:navigation-{start,end} events * changesets * Update .changeset/little-geckos-smell.md Co-authored-by: Ignatius Bagus * Update documentation/docs/08-events.md Co-authored-by: Ignatius Bagus * make navigation abortable * sigh. lint * ugh come on * Update packages/kit/src/runtime/client/router.js Co-authored-by: Maurício Kishi * belt and braces * ugh * changeset * for the love of * use pushState for redirects Co-authored-by: Ignatius Bagus Co-authored-by: Maurício Kishi --- .changeset/cyan-cobras-explode.md | 5 +++ .changeset/little-geckos-smell.md | 5 +++ .changeset/old-years-march.md | 5 +++ documentation/docs/08-events.md | 8 +--- packages/kit/src/runtime/client/renderer.js | 9 ++--- packages/kit/src/runtime/client/router.js | 20 +++++----- .../kit/src/runtime/server/page/load_node.js | 2 - packages/kit/test/ambient.d.ts | 1 + .../routing/cancellation/__layout.svelte | 4 ++ .../src/routes/routing/cancellation/a.svelte | 17 +++++++++ .../src/routes/routing/cancellation/b.svelte | 1 + .../routes/routing/cancellation/index.svelte | 1 + packages/kit/test/apps/basics/test/test.js | 33 ++++++++++++++--- packages/kit/test/utils.d.ts | 2 +- packages/kit/test/utils.js | 37 +++---------------- 15 files changed, 90 insertions(+), 60 deletions(-) create mode 100644 .changeset/cyan-cobras-explode.md create mode 100644 .changeset/little-geckos-smell.md create mode 100644 .changeset/old-years-march.md create mode 100644 packages/kit/test/apps/basics/src/routes/routing/cancellation/__layout.svelte create mode 100644 packages/kit/test/apps/basics/src/routes/routing/cancellation/a.svelte create mode 100644 packages/kit/test/apps/basics/src/routes/routing/cancellation/b.svelte create mode 100644 packages/kit/test/apps/basics/src/routes/routing/cancellation/index.svelte diff --git a/.changeset/cyan-cobras-explode.md b/.changeset/cyan-cobras-explode.md new file mode 100644 index 000000000000..fdb170084db8 --- /dev/null +++ b/.changeset/cyan-cobras-explode.md @@ -0,0 +1,5 @@ +--- +'@sveltejs/kit': patch +--- + +[breaking] referer header sent by fetch in load matches page's referer header, not the page itself diff --git a/.changeset/little-geckos-smell.md b/.changeset/little-geckos-smell.md new file mode 100644 index 000000000000..f7e2f421c7a0 --- /dev/null +++ b/.changeset/little-geckos-smell.md @@ -0,0 +1,5 @@ +--- +'@sveltejs/kit': patch +--- + +[breaking] remove sveltekit:navigation-{start,end} events diff --git a/.changeset/old-years-march.md b/.changeset/old-years-march.md new file mode 100644 index 000000000000..c51205ec9d8b --- /dev/null +++ b/.changeset/old-years-march.md @@ -0,0 +1,5 @@ +--- +'@sveltejs/kit': patch +--- + +[breaking] defer pushState until navigation occurs diff --git a/documentation/docs/08-events.md b/documentation/docs/08-events.md index 58d01a516ba6..205b399b30ca 100644 --- a/documentation/docs/08-events.md +++ b/documentation/docs/08-events.md @@ -2,10 +2,6 @@ title: Events --- -SvelteKit emits [CustomEvents](https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent) on the `window` object when certain things happen: +SvelteKit emits a `sveltekit:start` [CustomEvent](https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent) on the `window` object once the app has hydrated. -- `sveltekit:start` — fired once the app has hydrated -- `sveltekit:navigation-start` — navigation has started -- `sveltekit:navigation-end` — navigation has ended - -You probably won't need to use these, but they can be useful in the context of (for example) integration tests. +You probably won't need to use it, but it can be useful in the context of (for example) integration tests. diff --git a/packages/kit/src/runtime/client/renderer.js b/packages/kit/src/runtime/client/renderer.js index cc8c98212c17..3c0db40a66eb 100644 --- a/packages/kit/src/runtime/client/renderer.js +++ b/packages/kit/src/runtime/client/renderer.js @@ -335,11 +335,10 @@ export class Renderer { }); } else { if (this.router) { - this.router.goto( - new URL(navigation_result.redirect, info.url).href, - { replaceState: true }, - [...chain, info.url.pathname] - ); + this.router.goto(new URL(navigation_result.redirect, info.url).href, {}, [ + ...chain, + info.url.pathname + ]); } else { location.href = new URL(navigation_result.redirect, location.href).href; } diff --git a/packages/kit/src/runtime/client/router.js b/packages/kit/src/runtime/client/router.js index 53cb01de9b32..1f2351f35b70 100644 --- a/packages/kit/src/runtime/client/router.js +++ b/packages/kit/src/runtime/client/router.js @@ -425,20 +425,13 @@ export class Router { accepted(); - if (!this.navigating) { - dispatchEvent(new CustomEvent('sveltekit:navigation-start')); - } this.navigating++; const pathname = normalize_path(url.pathname, this.trailing_slash); info.url = new URL(url.origin + pathname + url.search + url.hash); - if (details) { - const change = details.replaceState ? 0 : 1; - details.state['sveltekit:index'] = this.current_history_index += change; - history[details.replaceState ? 'replaceState' : 'pushState'](details.state, '', info.url); - } + const token = (this.navigating_token = {}); await this.renderer.handle_navigation(info, chain, false, { scroll, @@ -446,11 +439,18 @@ export class Router { }); this.navigating--; - if (!this.navigating) { - dispatchEvent(new CustomEvent('sveltekit:navigation-end')); + // navigation was aborted + if (this.navigating_token !== token) return; + if (!this.navigating) { const navigation = { from, to: url }; this.callbacks.after_navigate.forEach((fn) => fn(navigation)); } + + if (details) { + const change = details.replaceState ? 0 : 1; + details.state['sveltekit:index'] = this.current_history_index += change; + history[details.replaceState ? 'replaceState' : 'pushState'](details.state, '', info.url); + } } } diff --git a/packages/kit/src/runtime/server/page/load_node.js b/packages/kit/src/runtime/server/page/load_node.js index 96bcfc641d89..841dc0f01779 100644 --- a/packages/kit/src/runtime/server/page/load_node.js +++ b/packages/kit/src/runtime/server/page/load_node.js @@ -139,8 +139,6 @@ export async function load_node({ } } - opts.headers.set('referer', event.url.href); - const resolved = resolve(event.url.pathname, requested.split('?')[0]); /** @type {Response} */ diff --git a/packages/kit/test/ambient.d.ts b/packages/kit/test/ambient.d.ts index ea7820b66d38..ed7ec4e0f548 100644 --- a/packages/kit/test/ambient.d.ts +++ b/packages/kit/test/ambient.d.ts @@ -6,6 +6,7 @@ declare global { // used in tests oops: string; pageContext: any; + fulfil_navigation: (value: any) => void; } const goto: ( diff --git a/packages/kit/test/apps/basics/src/routes/routing/cancellation/__layout.svelte b/packages/kit/test/apps/basics/src/routes/routing/cancellation/__layout.svelte new file mode 100644 index 000000000000..7a04256ede3d --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/routing/cancellation/__layout.svelte @@ -0,0 +1,4 @@ + + +a +b diff --git a/packages/kit/test/apps/basics/src/routes/routing/cancellation/a.svelte b/packages/kit/test/apps/basics/src/routes/routing/cancellation/a.svelte new file mode 100644 index 000000000000..ec4ec8478ebb --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/routing/cancellation/a.svelte @@ -0,0 +1,17 @@ + + +

this should not appear

\ No newline at end of file diff --git a/packages/kit/test/apps/basics/src/routes/routing/cancellation/b.svelte b/packages/kit/test/apps/basics/src/routes/routing/cancellation/b.svelte new file mode 100644 index 000000000000..51902b4da7a6 --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/routing/cancellation/b.svelte @@ -0,0 +1 @@ +

b

\ No newline at end of file diff --git a/packages/kit/test/apps/basics/src/routes/routing/cancellation/index.svelte b/packages/kit/test/apps/basics/src/routes/routing/cancellation/index.svelte new file mode 100644 index 000000000000..002578b92477 --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/routing/cancellation/index.svelte @@ -0,0 +1 @@ +

cancellation

\ No newline at end of file diff --git a/packages/kit/test/apps/basics/test/test.js b/packages/kit/test/apps/basics/test/test.js index 858659fb4ea8..d1203b732039 100644 --- a/packages/kit/test/apps/basics/test/test.js +++ b/packages/kit/test/apps/basics/test/test.js @@ -106,10 +106,10 @@ test.describe.parallel('beforeNavigate', () => { await page.goto('/before-navigate/prevent-navigation'); try { - await clicknav('[href="/before-navigate/a"]'); + await clicknav('[href="/before-navigate/a"]', { timeout: 1000 }); expect(false).toBe(true); } catch (/** @type {any} */ e) { - expect(e.message).toMatch('Timed out'); + expect(e.message).toMatch('page.waitForNavigation: Timeout 1000ms exceeded'); } expect(page.url()).toBe(baseURL + '/before-navigate/prevent-navigation'); @@ -1269,8 +1269,12 @@ test.describe.parallel('Load', () => { await clicknav('[href="/load/fetch-headers"]'); const json = /** @type {string} */ (await page.textContent('pre')); - expect(JSON.parse(json)).toEqual({ - referer: `${baseURL}/load/fetch-headers`, + const headers = JSON.parse(json); + + expect(headers).toEqual({ + // the referer will be the previous page in the client-side + // navigation case + referer: `${baseURL}/load`, // these headers aren't particularly useful, but they allow us to verify // that page headers are being forwarded 'sec-fetch-dest': javaScriptEnabled ? 'empty' : 'document', @@ -1631,13 +1635,17 @@ test.describe.parallel('searchParams', () => { }); test.describe.parallel('Redirects', () => { - test('redirect', async ({ page, clicknav }) => { + test('redirect', async ({ baseURL, page, clicknav, back }) => { await page.goto('/redirect'); await clicknav('[href="/redirect/a"]'); await page.waitForURL('/redirect/c'); expect(await page.textContent('h1')).toBe('c'); + expect(page.url()).toBe(`${baseURL}/redirect/c`); + + await back(); + expect(page.url()).toBe(`${baseURL}/redirect`); }); test('prevents redirect loops', async ({ baseURL, page, javaScriptEnabled }) => { @@ -2123,6 +2131,21 @@ test.describe.parallel('Routing', () => { await clicknav('[href="/static.json"]'); expect(await page.textContent('body')).toBe('"static file"\n'); }); + + test('navigation is cancelled upon subsequent navigation', async ({ + baseURL, + page, + clicknav + }) => { + await page.goto('/routing/cancellation'); + await page.click('[href="/routing/cancellation/a"]'); + await clicknav('[href="/routing/cancellation/b"]'); + + expect(await page.url()).toBe(`${baseURL}/routing/cancellation/b`); + + await page.evaluate('window.fulfil_navigation && window.fulfil_navigation()'); + expect(await page.url()).toBe(`${baseURL}/routing/cancellation/b`); + }); }); test.describe.parallel('Session', () => { diff --git a/packages/kit/test/utils.d.ts b/packages/kit/test/utils.d.ts index 194880ad7948..aea4c91489ca 100644 --- a/packages/kit/test/utils.d.ts +++ b/packages/kit/test/utils.d.ts @@ -20,7 +20,7 @@ export const test: TestType< prefetchRoutes: (urls: string[]) => Promise; }; back: () => Promise; - clicknav: (selector: string) => Promise; + clicknav: (selector: string, options?: { timeout?: number }) => Promise; in_view: (selector: string) => Promise; read_errors: (href: string) => string; }, diff --git a/packages/kit/test/utils.js b/packages/kit/test/utils.js index b5e4bbfff4c8..9fb8f1cd077d 100644 --- a/packages/kit/test/utils.js +++ b/packages/kit/test/utils.js @@ -58,20 +58,6 @@ export const test = base.extend({ back: async ({ page, javaScriptEnabled }, use) => { use(async () => { if (javaScriptEnabled) { - await page.evaluate(() => { - window.navigated = new Promise((fulfil, reject) => { - const timeout = setTimeout(() => reject(new Error('Timed out')), 2000); - addEventListener( - 'sveltekit:navigation-end', - () => { - clearTimeout(timeout); - fulfil(); - }, - { once: true } - ); - }); - }); - await Promise.all([page.goBack(), page.evaluate(() => window.navigated)]); } else { return page.goBack().then(() => void 0); @@ -81,24 +67,13 @@ export const test = base.extend({ // @ts-expect-error clicknav: async ({ page, javaScriptEnabled }, use) => { - /** @param {string} selector */ - async function clicknav(selector) { + /** + * @param {string} selector + * @param {{ timeout: number }} options + */ + async function clicknav(selector, options) { if (javaScriptEnabled) { - await page.evaluate(() => { - window.navigated = new Promise((fulfil, reject) => { - const timeout = setTimeout(() => reject(new Error('Timed out')), 2000); - addEventListener( - 'sveltekit:navigation-end', - () => { - clearTimeout(timeout); - fulfil(); - }, - { once: true } - ); - }); - }); - - await Promise.all([page.click(selector), page.evaluate(() => window.navigated)]); + await Promise.all([page.click(selector), page.waitForNavigation(options)]); } else { await page.click(selector); }