From 6e4a1bfddc7bd96959f05c70baac72be4a2b564b Mon Sep 17 00:00:00 2001 From: "S. Elliott Johnson" Date: Thu, 28 Apr 2022 20:49:21 -0600 Subject: [PATCH 01/13] feat: Added failing test --- .../routing/prefetched/hash-route.svelte | 27 +++++++++++++++++++ packages/kit/test/apps/basics/test/test.js | 13 +++++++++ 2 files changed, 40 insertions(+) create mode 100644 packages/kit/test/apps/basics/src/routes/routing/prefetched/hash-route.svelte diff --git a/packages/kit/test/apps/basics/src/routes/routing/prefetched/hash-route.svelte b/packages/kit/test/apps/basics/src/routes/routing/prefetched/hash-route.svelte new file mode 100644 index 000000000000..d5a5417148ec --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/routing/prefetched/hash-route.svelte @@ -0,0 +1,27 @@ + + + + +

+ {modal?.title ?? ''} +

diff --git a/packages/kit/test/apps/basics/test/test.js b/packages/kit/test/apps/basics/test/test.js index e9b0e2c71b9b..c02273c1ce3d 100644 --- a/packages/kit/test/apps/basics/test/test.js +++ b/packages/kit/test/apps/basics/test/test.js @@ -2040,6 +2040,19 @@ test.describe.parallel('Routing', () => { } }); + test('chooses correct route when hash route is prefetched but regular route is clicked', async ({ + app, + page, + javaScriptEnabled + }) => { + if (javaScriptEnabled) { + await page.goto('/routing/a'); + await app.prefetch('/routing/prefetched/hash-route#please-dont-show-me'); + await app.goto('/routing/prefetched/hash-route'); + await expect(page.locator('h1')).not.toHaveText('Oopsie'); + } + }); + test('does not attempt client-side navigation to server routes', async ({ page }) => { await page.goto('/routing'); await page.click('[href="/routing/ambiguous/ok.json"]'); From e536f644fc92e7c6f0761c8c4b2823ab11b40543 Mon Sep 17 00:00:00 2001 From: "S. Elliott Johnson" Date: Thu, 28 Apr 2022 21:45:54 -0600 Subject: [PATCH 02/13] fix: Prefetch weirdness with hash routes --- packages/kit/src/runtime/client/client.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index 9a4870b8b524..72070fc01dc1 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -838,7 +838,7 @@ export function create_client({ target, session, base, trailing_slash }) { if (params) { /** @type {import('./types').NavigationIntent} */ const intent = { - id: url.pathname + url.search, + id: url.pathname + url.hash + url.search, route, params, url From 6badadb060f1f46136fea6ff2f05a1bee0a4c006 Mon Sep 17 00:00:00 2001 From: "S. Elliott Johnson" Date: Thu, 28 Apr 2022 22:09:07 -0600 Subject: [PATCH 03/13] feat: Changeset --- .changeset/old-comics-happen.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/old-comics-happen.md diff --git a/.changeset/old-comics-happen.md b/.changeset/old-comics-happen.md new file mode 100644 index 000000000000..431678c2983c --- /dev/null +++ b/.changeset/old-comics-happen.md @@ -0,0 +1,5 @@ +--- +'@sveltejs/kit': patch +--- + +fix: sveltekit:prefetch correctly distinguishes between hash routes From 5dc424e521d93633e4aff2ded5759365f258c470 Mon Sep 17 00:00:00 2001 From: "S. Elliott Johnson" Date: Thu, 28 Apr 2022 23:18:08 -0600 Subject: [PATCH 04/13] fix: Remove useless log statement --- .../apps/basics/src/routes/routing/prefetched/hash-route.svelte | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/kit/test/apps/basics/src/routes/routing/prefetched/hash-route.svelte b/packages/kit/test/apps/basics/src/routes/routing/prefetched/hash-route.svelte index d5a5417148ec..35047709ba4d 100644 --- a/packages/kit/test/apps/basics/src/routes/routing/prefetched/hash-route.svelte +++ b/packages/kit/test/apps/basics/src/routes/routing/prefetched/hash-route.svelte @@ -12,7 +12,6 @@ let modal = undefined; const checkIfModalShouldBeShown = () => { - console.log('mount'); const modalToShow = $page.url.hash.substring(1); modal = modalContents.find(({ id }) => id === modalToShow); }; From da977dce14f305610b7f41d8ea4f852911b73a58 Mon Sep 17 00:00:00 2001 From: "S. Elliott Johnson" Date: Fri, 29 Apr 2022 01:37:13 -0600 Subject: [PATCH 05/13] feat: Tests to check for hash route prefetch reloads --- .../routing/prefetched/hash-route.svelte | 27 +++- packages/kit/test/apps/basics/test/test.js | 132 +++++++++++------- 2 files changed, 104 insertions(+), 55 deletions(-) diff --git a/packages/kit/test/apps/basics/src/routes/routing/prefetched/hash-route.svelte b/packages/kit/test/apps/basics/src/routes/routing/prefetched/hash-route.svelte index d5a5417148ec..99f6fa984c1c 100644 --- a/packages/kit/test/apps/basics/src/routes/routing/prefetched/hash-route.svelte +++ b/packages/kit/test/apps/basics/src/routes/routing/prefetched/hash-route.svelte @@ -1,20 +1,33 @@ + + From 86f757de3d48de8d691221e7c2cdfa00d397a3c6 Mon Sep 17 00:00:00 2001 From: "S. Elliott Johnson" Date: Sat, 30 Apr 2022 15:32:19 -0600 Subject: [PATCH 07/13] feat: Updated tests --- packages/kit/test/apps/basics/test/test.js | 24 ++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/packages/kit/test/apps/basics/test/test.js b/packages/kit/test/apps/basics/test/test.js index 9a5d4e023128..9098eba9725d 100644 --- a/packages/kit/test/apps/basics/test/test.js +++ b/packages/kit/test/apps/basics/test/test.js @@ -1945,7 +1945,7 @@ test.describe.parallel('Prefetching', () => { } }); - test('does rerun load on calls to different preload hash route', async ({ + test('does not rerun load on calls to different preload hash route', async ({ app, page, javaScriptEnabled @@ -1956,7 +1956,27 @@ test.describe.parallel('Prefetching', () => { await app.prefetch('/routing/prefetched/hash-route#please-dont-show-me'); await app.prefetch('/routing/prefetched/hash-route#please-dont-show-me-jr'); await app.goto('/routing/prefetched/hash-route#please-dont-show-me'); - await expect(page.locator('p')).toHaveText('Loaded 3 times.'); + await expect(page.locator('p')).toHaveText('Loaded 1 times.'); + } + }); + + test('successfully preloads same route with different hashes many times', async ({ + app, + page, + javaScriptEnabled + }) => { + // With the current implementation, I could maybe see some sort of stack overflow + // if the same route with different hashes were preloaded many many times, so + // just making sure we can preload a route a reasonable amount of times without any + // issues. + + if (javaScriptEnabled) { + await page.goto('/routing/a'); + + for (let i = 0; i < 1000; i++) { + await app.prefetch('/routing/prefetched/hash-route#please-dont-show-me'); + await app.prefetch('/routing/prefetched/hash-route#please-dont-show-me-jr'); + } } }); }); From 38ff71b5f06bc4c939ec0e10db16c7af12a5ddab Mon Sep 17 00:00:00 2001 From: "S. Elliott Johnson" Date: Sat, 30 Apr 2022 15:48:52 -0600 Subject: [PATCH 08/13] feat: Prerendered hash routes correctly pushed to page store - NavigationResultCache became complicated, so moved it to ./types.ts - If cache ID is the same, check the hash and port for changes. Update the cached URL if nothing but the hash has changed. --- packages/kit/src/runtime/client/client.js | 38 +++++++++++++++++++--- packages/kit/src/runtime/client/types.d.ts | 6 ++++ 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index 72070fc01dc1..f041cf897ae4 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -72,9 +72,10 @@ export function create_client({ target, session, base, trailing_slash }) { updated: create_updated_store() }; - /** @type {{id: string | null, promise: Promise | null}} */ + /** @type {import('./types').NavigationResultCache} */ const load_cache = { id: null, + url: null, promise: null }; @@ -194,6 +195,7 @@ export function create_client({ target, session, base, trailing_slash }) { } load_cache.promise = load_route(intent, false); + load_cache.url = intent.url; load_cache.id = intent.id; return load_cache.promise; @@ -331,6 +333,7 @@ export function create_client({ target, session, base, trailing_slash }) { } load_cache.promise = null; + load_cache.url = null; load_cache.id = null; autoscroll = true; updating = false; @@ -582,8 +585,9 @@ export function create_client({ target, session, base, trailing_slash }) { * @param {boolean} no_cache */ async function load_route({ id, url, params, route }, no_cache) { - if (load_cache.id === id && load_cache.promise) { - return load_cache.promise; + const validated_cache = validate_cache({ id, url, params, route }); + if (validated_cache !== null) { + return validated_cache; } if (!no_cache) { @@ -838,7 +842,7 @@ export function create_client({ target, session, base, trailing_slash }) { if (params) { /** @type {import('./types').NavigationIntent} */ const intent = { - id: url.pathname + url.hash + url.search, + id: url.pathname + url.search, route, params, url @@ -849,6 +853,32 @@ export function create_client({ target, session, base, trailing_slash }) { } } + /** + * @param {import('./types').NavigationIntent} intent + * @returns {Promise | null} + */ + function validate_cache({ id, url }) { + if (load_cache.id === id && load_cache.promise) { + // If the hash is the same, the cache is fully valid + if (load_cache?.url?.hash !== url.hash && load_cache?.url?.port === url.port) { + // If the hash is not the same, we know the URL is the same except for + // the hash, so replace the URL in the props with the new one. + // and leave everything else the same. + load_cache.promise = (async () => { + const nav_result = await load_cache.promise; + if (nav_result?.props?.page?.url) { + nav_result.props.page.url.hash = url.hash; + } + return nav_result ?? undefined; + })(); + } + + return load_cache.promise; + } + + return null; + } + /** * @param {{ * url: URL; diff --git a/packages/kit/src/runtime/client/types.d.ts b/packages/kit/src/runtime/client/types.d.ts index 4f5f6647bd38..f0996f1a7e79 100644 --- a/packages/kit/src/runtime/client/types.d.ts +++ b/packages/kit/src/runtime/client/types.d.ts @@ -75,3 +75,9 @@ export type NavigationState = { stuff: Record; url: URL; }; + +export type NavigationResultCache = { + id: string | null; + url: URL | null; + promise: Promise | null; +}; From 80c1a22106f13956308912cdd1040486e3182292 Mon Sep 17 00:00:00 2001 From: "S. Elliott Johnson" Date: Sat, 30 Apr 2022 18:12:50 -0600 Subject: [PATCH 09/13] chore: Update changelog --- .changeset/old-comics-happen.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/old-comics-happen.md b/.changeset/old-comics-happen.md index 431678c2983c..57449afc3474 100644 --- a/.changeset/old-comics-happen.md +++ b/.changeset/old-comics-happen.md @@ -2,4 +2,4 @@ '@sveltejs/kit': patch --- -fix: sveltekit:prefetch correctly distinguishes between hash routes +fix: page store correct after navigation when an identical route with a different hash had been prefetched From 6f73973fd0f6c5a2d197a7b1b0ee49ed4261d664 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Mon, 23 May 2022 17:36:54 -0400 Subject: [PATCH 10/13] reset props.url before updating page --- packages/kit/src/runtime/client/client.js | 182 +++++++++++++++------- 1 file changed, 123 insertions(+), 59 deletions(-) diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index f041cf897ae4..7576c7db9693 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -48,6 +48,33 @@ function update_scroll_positions(index) { scroll_positions[index] = scroll_state(); } +const fetch = window.fetch; +let loading = 0; + +if (import.meta.env.DEV) { + let can_inspect_stack_trace = false; + + const check_stack_trace = async () => { + const stack = /** @type {string} */ (new Error().stack); + can_inspect_stack_trace = stack.includes('check_stack_trace'); + }; + + check_stack_trace(); + + window.fetch = (input, init) => { + const url = input instanceof Request ? input.url : input.toString(); + const stack = /** @type {string} */ (new Error().stack); + + const heuristic = can_inspect_stack_trace ? stack.includes('load_node') : loading; + if (heuristic) { + console.warn( + `Loading ${url} using \`window.fetch\`. For best results, use the \`fetch\` that is passed to your \`load\` function: https://kit.svelte.dev/docs/loading#input-fetch` + ); + } + return fetch(input, init); + }; +} + /** * @param {{ * target: Element; @@ -72,10 +99,9 @@ export function create_client({ target, session, base, trailing_slash }) { updated: create_updated_store() }; - /** @type {import('./types').NavigationResultCache} */ + /** @type {{id: string | null, promise: Promise | null}} */ const load_cache = { id: null, - url: null, promise: null }; @@ -195,7 +221,6 @@ export function create_client({ target, session, base, trailing_slash }) { } load_cache.promise = load_route(intent, false); - load_cache.url = intent.url; load_cache.id = intent.id; return load_cache.promise; @@ -207,8 +232,9 @@ export function create_client({ target, session, base, trailing_slash }) { * @param {string[]} redirect_chain * @param {boolean} no_cache * @param {{hash?: string, scroll: { x: number, y: number } | null, keepfocus: boolean, details: { replaceState: boolean, state: any } | null}} [opts] + * @param {() => void} [callback] */ - async function update(url, redirect_chain, no_cache, opts) { + async function update(url, redirect_chain, no_cache, opts, callback) { const intent = get_navigation_intent(url); const current_token = (token = {}); @@ -281,6 +307,10 @@ export function create_client({ target, session, base, trailing_slash }) { if (started) { current = navigation_result.state; + if (navigation_result.props.page) { + navigation_result.props.page.url = url; + } + root.$set(navigation_result.props); } else { initialize(navigation_result); @@ -301,7 +331,7 @@ export function create_client({ target, session, base, trailing_slash }) { getSelection()?.removeAllRanges(); root.tabIndex = -1; - root.focus(); + root.focus({ preventScroll: true }); // restore `tabindex` as to prevent `root` from stealing input from elements if (tabindex !== null) { @@ -333,10 +363,8 @@ export function create_client({ target, session, base, trailing_slash }) { } load_cache.promise = null; - load_cache.url = null; load_cache.id = null; autoscroll = true; - updating = false; if (navigation_result.props.page) { page = navigation_result.props.page; @@ -345,7 +373,9 @@ export function create_client({ target, session, base, trailing_slash }) { const leaf_node = navigation_result.state.branch[navigation_result.state.branch.length - 1]; router_enabled = leaf_node?.module.router !== false; - return true; + if (callback) callback(); + + updating = false; } /** @param {import('./types').NavigationResult} result */ @@ -363,12 +393,12 @@ export function create_client({ target, session, base, trailing_slash }) { hydrate: true }); - started = true; - if (router_enabled) { const navigation = { from: null, to: new URL(location.href) }; callbacks.after_navigate.forEach((fn) => fn(navigation)); } + + started = true; } /** @@ -526,14 +556,25 @@ export function create_client({ target, session, base, trailing_slash }) { const session = $session; if (module.load) { - /** @type {import('types').LoadInput} */ + /** @type {import('types').LoadEvent} */ const load_input = { routeId, params: uses_params, props: props || {}, get url() { node.uses.url = true; - return url; + + return new Proxy(url, { + get: (target, property) => { + if (property === 'hash') { + throw new Error( + 'url.hash is inaccessible from load. Consider accessing hash from the page store within the script tag of your component.' + ); + } + + return Reflect.get(target, property, target); + } + }); }, get session() { node.uses.session = true; @@ -543,11 +584,44 @@ export function create_client({ target, session, base, trailing_slash }) { node.uses.stuff = true; return { ...stuff }; }, - fetch(resource, info) { - const requested = typeof resource === 'string' ? resource : resource.url; - add_dependency(requested); + async fetch(resource, init) { + let requested; + + if (typeof resource === 'string') { + requested = resource; + } else { + requested = resource.url; + + // we're not allowed to modify the received `Request` object, so in order + // to fixup relative urls we create a new equivalent `init` object instead + init = { + // the request body must be consumed in memory until browsers + // implement streaming request bodies and/or the body getter + body: + resource.method === 'GET' || resource.method === 'HEAD' + ? undefined + : await resource.blob(), + cache: resource.cache, + credentials: resource.credentials, + headers: resource.headers, + integrity: resource.integrity, + keepalive: resource.keepalive, + method: resource.method, + mode: resource.mode, + redirect: resource.redirect, + referrer: resource.referrer, + referrerPolicy: resource.referrerPolicy, + signal: resource.signal, + ...init + }; + } + + // we must fixup relative urls so they are resolved from the target page + const normalized = new URL(requested, url).href; + add_dependency(normalized); - return started ? fetch(resource, info) : initial_fetch(resource, info); + // prerendered pages may be served from any origin, so `initial_fetch` urls shouldn't be normalized + return started ? fetch(normalized, init) : initial_fetch(requested, init); }, status: status ?? null, error: error ?? null @@ -562,7 +636,18 @@ export function create_client({ target, session, base, trailing_slash }) { }); } - const loaded = await module.load.call(null, load_input); + let loaded; + + if (import.meta.env.DEV) { + try { + loading += 1; + loaded = await module.load.call(null, load_input); + } finally { + loading -= 1; + } + } else { + loaded = await module.load.call(null, load_input); + } if (!loaded) { throw new Error('load function must return a value'); @@ -585,9 +670,8 @@ export function create_client({ target, session, base, trailing_slash }) { * @param {boolean} no_cache */ async function load_route({ id, url, params, route }, no_cache) { - const validated_cache = validate_cache({ id, url, params, route }); - if (validated_cache !== null) { - return validated_cache; + if (load_cache.id === id && load_cache.promise) { + return load_cache.promise; } if (!no_cache) { @@ -616,8 +700,10 @@ export function create_client({ target, session, base, trailing_slash }) { /** @type {Error | null} */ let error = null; - // preload modules - a.forEach((loader) => loader()); + // preload modules to avoid waterfall, but handle rejections + // so they don't get reported to Sentry et al (we don't need + // to act on the failures at this point) + a.forEach((loader) => loader().catch(() => {})); load: for (let i = 0; i < a.length; i += 1) { /** @type {import('./types').BranchNode | undefined} */ @@ -853,32 +939,6 @@ export function create_client({ target, session, base, trailing_slash }) { } } - /** - * @param {import('./types').NavigationIntent} intent - * @returns {Promise | null} - */ - function validate_cache({ id, url }) { - if (load_cache.id === id && load_cache.promise) { - // If the hash is the same, the cache is fully valid - if (load_cache?.url?.hash !== url.hash && load_cache?.url?.port === url.port) { - // If the hash is not the same, we know the URL is the same except for - // the hash, so replace the URL in the props with the new one. - // and leave everything else the same. - load_cache.promise = (async () => { - const nav_result = await load_cache.promise; - if (nav_result?.props?.page?.url) { - nav_result.props.page.url.hash = url.hash; - } - return nav_result ?? undefined; - })(); - } - - return load_cache.promise; - } - - return null; - } - /** * @param {{ * url: URL; @@ -924,18 +984,22 @@ export function create_client({ target, session, base, trailing_slash }) { }); } - const completed = await update(normalized, redirect_chain, false, { - scroll, - keepfocus, - details - }); - - if (completed) { - const navigation = { from, to: normalized }; - callbacks.after_navigate.forEach((fn) => fn(navigation)); + await update( + normalized, + redirect_chain, + false, + { + scroll, + keepfocus, + details + }, + () => { + const navigation = { from, to: normalized }; + callbacks.after_navigate.forEach((fn) => fn(navigation)); - stores.navigating.set(null); - } + stores.navigating.set(null); + } + ); } /** From bb35c72fbbaeaba7cea294ac0fa96adb667e909b Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Mon, 23 May 2022 17:40:22 -0400 Subject: [PATCH 11/13] remove unused type --- packages/kit/src/runtime/client/types.d.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/packages/kit/src/runtime/client/types.d.ts b/packages/kit/src/runtime/client/types.d.ts index f0996f1a7e79..4f5f6647bd38 100644 --- a/packages/kit/src/runtime/client/types.d.ts +++ b/packages/kit/src/runtime/client/types.d.ts @@ -75,9 +75,3 @@ export type NavigationState = { stuff: Record; url: URL; }; - -export type NavigationResultCache = { - id: string | null; - url: URL | null; - promise: Promise | null; -}; From 947ded0f5542ad29bd464fa4ed1ace93b823badb Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Mon, 23 May 2022 17:43:10 -0400 Subject: [PATCH 12/13] use snake_case etc for codebase consistency --- .../routing/prefetched/hash-route.svelte | 27 ++++++++----------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/packages/kit/test/apps/basics/src/routes/routing/prefetched/hash-route.svelte b/packages/kit/test/apps/basics/src/routes/routing/prefetched/hash-route.svelte index 92f023ca8815..39fa611fe77e 100644 --- a/packages/kit/test/apps/basics/src/routes/routing/prefetched/hash-route.svelte +++ b/packages/kit/test/apps/basics/src/routes/routing/prefetched/hash-route.svelte @@ -1,9 +1,9 @@ @@ -13,7 +13,7 @@ export let calls; - const modalContents = { + const modal_contents = { 'please-dont-show-me': { title: 'Oopsie' }, @@ -24,20 +24,15 @@ let modal = undefined; - const checkIfModalShouldBeShown = () => { - const modalToShow = $page.url.hash.substring(1); - modal = modalContents[modalToShow]; + const show_modal = () => { + const hash = $page.url.hash.substring(1); + modal = modal_contents[hash]; }; - onMount(checkIfModalShouldBeShown); + onMount(show_modal); - + -

- {modal?.title ?? ''} -

- -

- Loaded {calls} times. -

+

{modal?.title ?? ''}

+

Loaded {calls} times.

From 03a4ed8d6d8a8b8098cb7ea1d94a62b554c14118 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Mon, 23 May 2022 17:50:49 -0400 Subject: [PATCH 13/13] remove unnecessary test --- packages/kit/test/apps/basics/test/test.js | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/packages/kit/test/apps/basics/test/test.js b/packages/kit/test/apps/basics/test/test.js index cc173c6b48b1..755f9131c032 100644 --- a/packages/kit/test/apps/basics/test/test.js +++ b/packages/kit/test/apps/basics/test/test.js @@ -2069,26 +2069,6 @@ test.describe.parallel('Prefetching', () => { await expect(page.locator('p')).toHaveText('Loaded 1 times.'); } }); - - test('successfully preloads same route with different hashes many times', async ({ - app, - page, - javaScriptEnabled - }) => { - // With the current implementation, I could maybe see some sort of stack overflow - // if the same route with different hashes were preloaded many many times, so - // just making sure we can preload a route a reasonable amount of times without any - // issues. - - if (javaScriptEnabled) { - await page.goto('/routing/a'); - - for (let i = 0; i < 1000; i++) { - await app.prefetch('/routing/prefetched/hash-route#please-dont-show-me'); - await app.prefetch('/routing/prefetched/hash-route#please-dont-show-me-jr'); - } - } - }); }); test.describe.parallel('Routing', () => {