From 04095cba110a5d7142b71f4aa1ab79222276e152 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 18 Aug 2022 17:42:09 -0400 Subject: [PATCH 01/19] failing tests for #5960 --- .../routes/load/unchanged/+layout.server.js | 7 ++++ .../src/routes/load/unchanged/+layout.svelte | 2 ++ .../src/routes/load/unchanged/+page.svelte | 2 ++ .../unchanged/isolated/[slug]/+page.server.js | 6 ++++ .../unchanged/isolated/[slug]/+page.svelte | 7 ++++ .../routes/load/unchanged/reset/+server.js | 5 +++ .../basics/src/routes/load/unchanged/state.js | 9 +++++ .../uses-parent/[slug]/+page.server.js | 9 +++++ .../unchanged/uses-parent/[slug]/+page.svelte | 8 +++++ packages/kit/test/apps/basics/test/test.js | 34 +++++++++++++++++++ 10 files changed, 89 insertions(+) create mode 100644 packages/kit/test/apps/basics/src/routes/load/unchanged/+layout.server.js create mode 100644 packages/kit/test/apps/basics/src/routes/load/unchanged/+layout.svelte create mode 100644 packages/kit/test/apps/basics/src/routes/load/unchanged/+page.svelte create mode 100644 packages/kit/test/apps/basics/src/routes/load/unchanged/isolated/[slug]/+page.server.js create mode 100644 packages/kit/test/apps/basics/src/routes/load/unchanged/isolated/[slug]/+page.svelte create mode 100644 packages/kit/test/apps/basics/src/routes/load/unchanged/reset/+server.js create mode 100644 packages/kit/test/apps/basics/src/routes/load/unchanged/state.js create mode 100644 packages/kit/test/apps/basics/src/routes/load/unchanged/uses-parent/[slug]/+page.server.js create mode 100644 packages/kit/test/apps/basics/src/routes/load/unchanged/uses-parent/[slug]/+page.svelte diff --git a/packages/kit/test/apps/basics/src/routes/load/unchanged/+layout.server.js b/packages/kit/test/apps/basics/src/routes/load/unchanged/+layout.server.js new file mode 100644 index 000000000000..3e71bd890ab4 --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/load/unchanged/+layout.server.js @@ -0,0 +1,7 @@ +import { increment } from './state.js'; + +export function load() { + return { + count: increment() + }; +} diff --git a/packages/kit/test/apps/basics/src/routes/load/unchanged/+layout.svelte b/packages/kit/test/apps/basics/src/routes/load/unchanged/+layout.svelte new file mode 100644 index 000000000000..2a4c2dcdf06c --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/load/unchanged/+layout.svelte @@ -0,0 +1,2 @@ + + diff --git a/packages/kit/test/apps/basics/src/routes/load/unchanged/+page.svelte b/packages/kit/test/apps/basics/src/routes/load/unchanged/+page.svelte new file mode 100644 index 000000000000..92cd56efc69b --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/load/unchanged/+page.svelte @@ -0,0 +1,2 @@ +uses parent +isolated diff --git a/packages/kit/test/apps/basics/src/routes/load/unchanged/isolated/[slug]/+page.server.js b/packages/kit/test/apps/basics/src/routes/load/unchanged/isolated/[slug]/+page.server.js new file mode 100644 index 000000000000..e17baea223d1 --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/load/unchanged/isolated/[slug]/+page.server.js @@ -0,0 +1,6 @@ +/** @type {import('./$types').PageData} */ +export function load({ params }) { + return { + slug: params.slug + }; +} diff --git a/packages/kit/test/apps/basics/src/routes/load/unchanged/isolated/[slug]/+page.svelte b/packages/kit/test/apps/basics/src/routes/load/unchanged/isolated/[slug]/+page.svelte new file mode 100644 index 000000000000..b04d74c9430f --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/load/unchanged/isolated/[slug]/+page.svelte @@ -0,0 +1,7 @@ + + +

slug: {data.slug}

+

count: {data.count}

diff --git a/packages/kit/test/apps/basics/src/routes/load/unchanged/reset/+server.js b/packages/kit/test/apps/basics/src/routes/load/unchanged/reset/+server.js new file mode 100644 index 000000000000..147439045bf7 --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/load/unchanged/reset/+server.js @@ -0,0 +1,5 @@ +import { reset } from '../state.js'; + +export function POST() { + reset(); +} diff --git a/packages/kit/test/apps/basics/src/routes/load/unchanged/state.js b/packages/kit/test/apps/basics/src/routes/load/unchanged/state.js new file mode 100644 index 000000000000..10436c57304a --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/load/unchanged/state.js @@ -0,0 +1,9 @@ +let count = 0; + +export function increment() { + return count++; +} + +export function reset() { + count = 0; +} diff --git a/packages/kit/test/apps/basics/src/routes/load/unchanged/uses-parent/[slug]/+page.server.js b/packages/kit/test/apps/basics/src/routes/load/unchanged/uses-parent/[slug]/+page.server.js new file mode 100644 index 000000000000..7bc7b8df34d8 --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/load/unchanged/uses-parent/[slug]/+page.server.js @@ -0,0 +1,9 @@ +/** @type {import('./$types').PageData} */ +export async function load({ params, parent }) { + const { count } = await parent(); + + return { + doubled: count * 2, + slug: params.slug + }; +} diff --git a/packages/kit/test/apps/basics/src/routes/load/unchanged/uses-parent/[slug]/+page.svelte b/packages/kit/test/apps/basics/src/routes/load/unchanged/uses-parent/[slug]/+page.svelte new file mode 100644 index 000000000000..f23520ab163b --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/load/unchanged/uses-parent/[slug]/+page.svelte @@ -0,0 +1,8 @@ + + +

slug: {data.slug}

+

count: {data.count}

+

doubled: {data.doubled}

diff --git a/packages/kit/test/apps/basics/test/test.js b/packages/kit/test/apps/basics/test/test.js index 1a0f1a553bd7..0f917ab27c81 100644 --- a/packages/kit/test/apps/basics/test/test.js +++ b/packages/kit/test/apps/basics/test/test.js @@ -878,6 +878,40 @@ test.describe('Load', () => { }) ).toBe('rgb(255, 0, 0)'); }); + + test.only('+layout.server.js does not re-run when downstream load functions are invalidated', async ({ + page, + request, + clicknav + }) => { + await request.post('/load/unchanged/reset'); + + await page.goto('/load/unchanged/isolated/a'); + expect(await page.textContent('h1')).toBe('slug: a'); + expect(await page.textContent('h2')).toBe('count: 0'); + + await clicknav('[href="/load/unchanged/isolated/b"]'); + expect(await page.textContent('h1')).toBe('slug: b'); + expect(await page.textContent('h2')).toBe('count: 0'); + }); + + test.only('+layout.server.js re-runs when await parent() is called from downstream load function', async ({ + page, + request, + clicknav + }) => { + await request.post('/load/unchanged/reset'); + + await page.goto('/load/unchanged/with-parent/a'); + expect(await page.textContent('h1')).toBe('slug: a'); + expect(await page.textContent('h2')).toBe('count: 0'); + expect(await page.textContent('h3')).toBe('doubled: 0'); + + await clicknav('[href="/load/unchanged/with-parent/b"]'); + expect(await page.textContent('h1')).toBe('slug: b'); + expect(await page.textContent('h2')).toBe('count: 0'); + expect(await page.textContent('h3')).toBe('doubled: 2'); + }); }); test.describe('Method overrides', () => { From 646a884e6e9922febc66524f37bc6dcbb8b9e703 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 18 Aug 2022 18:39:49 -0400 Subject: [PATCH 02/19] explanatory note --- packages/kit/test/apps/basics/test/test.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/kit/test/apps/basics/test/test.js b/packages/kit/test/apps/basics/test/test.js index 72ebe1c5045d..46ec336b5446 100644 --- a/packages/kit/test/apps/basics/test/test.js +++ b/packages/kit/test/apps/basics/test/test.js @@ -907,6 +907,8 @@ test.describe('Load', () => { await clicknav('[href="/load/unchanged/with-parent/b"]'); expect(await page.textContent('h1')).toBe('slug: b'); expect(await page.textContent('h2')).toBe('count: 0'); + + // this looks wrong, but is actually the intended behaviour (the increment side-effect in a GET would be a bug in a real app) expect(await page.textContent('h3')).toBe('doubled: 2'); }); From 14e8f64eb98ecef5a825ead93a84ccb947c98bdc Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 18 Aug 2022 22:26:02 -0400 Subject: [PATCH 03/19] track dependencies --- .../kit/src/runtime/server/page/load_data.js | 50 ++++++++++++++++--- 1 file changed, 44 insertions(+), 6 deletions(-) diff --git a/packages/kit/src/runtime/server/page/load_data.js b/packages/kit/src/runtime/server/page/load_data.js index 2562356fd0c3..3f547906fe23 100644 --- a/packages/kit/src/runtime/server/page/load_data.js +++ b/packages/kit/src/runtime/server/page/load_data.js @@ -12,15 +12,45 @@ import { LoadURL, PrerenderingURL } from '../../../utils/url.js'; export async function load_server_data({ dev, event, node, parent }) { if (!node?.server) return null; - const server_data = await node.server.load?.call(null, { + const uses = { + dependencies: new Set(), + params: new Set(), + parent: false, + url: false + }; + + /** @param {string[]} deps */ + function depends(...deps) { + for (const dep of deps) { + const { href } = new URL(dep, event.url); + uses.dependencies.add(href); + } + } + + const params = new Proxy(event.params, { + get: (target, key) => { + if (key in target) { + uses.params.add(key); + return target[/** @type {string} */ (key)]; + } + + return undefined; + } + }); + + const result = await node.server.load?.call(null, { // can't use destructuring here because it will always // invoke event.clientAddress, which breaks prerendering get clientAddress() { return event.clientAddress; }, + depends, locals: event.locals, - params: event.params, - parent, + params, + parent: async () => { + uses.parent = true; + return parent(); + }, platform: event.platform, request: event.request, routeId: event.routeId, @@ -28,13 +58,21 @@ export async function load_server_data({ dev, event, node, parent }) { url: event.url }); - const result = server_data ? await unwrap_promises(server_data) : null; + const data = result ? await unwrap_promises(result) : null; if (dev) { - check_serializability(result, /** @type {string} */ (node.server_id), 'data'); + check_serializability(data, /** @type {string} */ (node.server_id), 'data'); } - return result; + return { + data, + uses: { + dependencies: Array.from(uses.dependencies), + params: Array.from(uses.params), + parent: uses.parent, + url: uses.url + } + }; } /** From f7e0af651515d7e27c98a3f9f916ce091cc1697d Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 18 Aug 2022 23:03:35 -0400 Subject: [PATCH 04/19] some progress --- packages/kit/src/runtime/client/client.js | 13 ++-- packages/kit/src/runtime/client/types.d.ts | 6 +- packages/kit/src/runtime/server/index.js | 78 ++++++++++--------- packages/kit/src/runtime/server/page/index.js | 4 +- .../kit/src/runtime/server/page/load_data.js | 14 ++-- .../unchanged/isolated/[slug]/+page.server.js | 2 +- .../uses-parent/[slug]/+page.server.js | 2 +- packages/kit/test/apps/basics/test/test.js | 4 +- 8 files changed, 66 insertions(+), 57 deletions(-) diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index 08a0e2de083c..b482a1f06d2d 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -498,7 +498,7 @@ export function create_client({ target, base, trailing_slash }) { const load_input = { routeId, params: uses_params, - data: server_data, + data: server_data ?? null, get url() { uses.url = true; return load_url; @@ -667,8 +667,8 @@ export function create_client({ target, base, trailing_slash }) { if (changed_since_last_render) { const payload = server_data_nodes?.[i]; - if (payload?.status) { - throw error(payload.status, payload.message); + if (payload?.httperror) { + throw error(payload.httperror.status, payload.httperror.message); } if (payload?.error) { @@ -1199,10 +1199,13 @@ export function create_client({ target, base, trailing_slash }) { const script = document.querySelector(`script[sveltekit\\:data-type="${type}"]`); return script?.textContent ? JSON.parse(script.textContent) : fallback; }; - const server_data = parse('server_data', []); + const all_server_data = parse('server_data', []); const validation_errors = parse('validation_errors', undefined); const branch_promises = node_ids.map(async (n, i) => { + // TODO handle case where this contains an "error" or "httperror" + const server_data = all_server_data[i]; + return load_node({ node: await nodes[n](), url, @@ -1215,7 +1218,7 @@ export function create_client({ target, base, trailing_slash }) { } return data; }, - server_data: server_data[i] ?? null + server_data: server_data?.data ?? null }); }); diff --git a/packages/kit/src/runtime/client/types.d.ts b/packages/kit/src/runtime/client/types.d.ts index bfcbc3dd59c8..651d6ecd70c2 100644 --- a/packages/kit/src/runtime/client/types.d.ts +++ b/packages/kit/src/runtime/client/types.d.ts @@ -93,8 +93,10 @@ export interface ServerDataLoaded { type: 'data'; nodes: Array<{ data?: Record | null; // TODO or `-1` to indicate 'reuse cached data'? - status?: number; - message?: string; + httperror?: { + status: number; + message: string; + }; error?: { name: string; message: string; diff --git a/packages/kit/src/runtime/server/index.js b/packages/kit/src/runtime/server/index.js index e4a8bd422dcf..dc843f796df3 100644 --- a/packages/kit/src/runtime/server/index.js +++ b/packages/kit/src/runtime/server/index.js @@ -254,58 +254,62 @@ export async function respond(request, options, state) { let response; if (is_data_request && route.type === 'page') { try { - /** @type {Redirect | HttpError | Error} */ - let error; + let aborted = false; - // TODO only get the data we need for the navigation const promises = [...route.layouts, route.leaf].map(async (n, i) => { try { - if (error) return; + if (aborted) return; // == because it could be undefined (in dev) or null (in build, because of JSON.stringify) const node = n == undefined ? n : await options.manifest._.nodes[n](); - return { - // TODO return `uses`, so we can reuse server data effectively - data: await load_server_data({ - dev: options.dev, - event, - node, - parent: async () => { - /** @type {Record} */ - const data = {}; - for (let j = 0; j < i; j += 1) { - const parent = await promises[j]; - if (!parent || parent instanceof HttpError || 'error' in parent) { - return data; - } - Object.assign(data, parent.data); - } - return data; + return await load_server_data({ + dev: options.dev, + event, + node, + parent: async () => { + /** @type {Record} */ + const data = {}; + for (let j = 0; j < i; j += 1) { + const parent = await promises[j]; + Object.assign(data, parent.data); } - }) - }; + return data; + } + }); } catch (e) { - error = normalize_error(e); + aborted = true; + throw e; + } + }); - if (error instanceof Redirect) { - throw error; - } + let length = promises.length; + const nodes = await Promise.all( + promises.map((p, i) => + p.catch((e) => { + const error = normalize_error(e); - if (error instanceof HttpError) { - return error; // { status, message } - } + if (error instanceof Redirect) { + throw error; + } - options.handle_error(error, event); + length = Math.min(length, i + 1); - return { - error: error_to_pojo(error, options.get_stack) - }; - } - }); + if (error instanceof HttpError) { + return { httperror: error }; + } + + options.handle_error(error, event); + + return { + error: error_to_pojo(error, options.get_stack) + }; + }) + ) + ); response = json({ type: 'data', - nodes: await Promise.all(promises) + nodes: nodes.slice(0, length) }); } catch (e) { const error = normalize_error(e); diff --git a/packages/kit/src/runtime/server/page/index.js b/packages/kit/src/runtime/server/page/index.js index 07ccefbe7ace..09533cb88be6 100644 --- a/packages/kit/src/runtime/server/page/index.js +++ b/packages/kit/src/runtime/server/page/index.js @@ -145,7 +145,6 @@ export async function render_page(event, route, options, state, resolve_opts) { /** @type {Error | null} */ let load_error = null; - /** @type {Array | null>>} */ const server_promises = nodes.map((node, i) => { if (load_error) { // if an error happens immediately, don't bother with the rest of the nodes @@ -168,7 +167,8 @@ export async function render_page(event, route, options, state, resolve_opts) { /** @type {Record} */ const data = {}; for (let j = 0; j < i; j += 1) { - Object.assign(data, await server_promises[j]); + const parent = await server_promises[j]; + Object.assign(data, await parent.data); } return data; } diff --git a/packages/kit/src/runtime/server/page/load_data.js b/packages/kit/src/runtime/server/page/load_data.js index 3f547906fe23..8d2f378d94d5 100644 --- a/packages/kit/src/runtime/server/page/load_data.js +++ b/packages/kit/src/runtime/server/page/load_data.js @@ -67,10 +67,10 @@ export async function load_server_data({ dev, event, node, parent }) { return { data, uses: { - dependencies: Array.from(uses.dependencies), - params: Array.from(uses.params), - parent: uses.parent, - url: uses.url + dependencies: uses.dependencies.size > 0 ? Array.from(uses.dependencies) : undefined, + params: uses.params.size > 0 ? Array.from(uses.params) : undefined, + parent: uses.parent ? 1 : undefined, + url: uses.url ? 1 : undefined } }; } @@ -82,7 +82,7 @@ export async function load_server_data({ dev, event, node, parent }) { * fetcher: typeof fetch; * node: import('types').SSRNode | undefined; * parent: () => Promise>; - * server_data_promise: Promise | null>; + * server_data_promise: Promise<{ data: Record } | null>; * state: import('types').SSRState; * }} opts */ @@ -90,13 +90,13 @@ export async function load_data({ event, fetcher, node, parent, server_data_prom const server_data = await server_data_promise; if (!node?.shared?.load) { - return server_data; + return server_data?.data; } const data = await node.shared.load.call(null, { url: state.prerendering ? new PrerenderingURL(event.url) : new LoadURL(event.url), params: event.params, - data: server_data, + data: server_data?.data ?? null, routeId: event.routeId, get session() { // TODO remove for 1.0 diff --git a/packages/kit/test/apps/basics/src/routes/load/unchanged/isolated/[slug]/+page.server.js b/packages/kit/test/apps/basics/src/routes/load/unchanged/isolated/[slug]/+page.server.js index e17baea223d1..ca49098363d1 100644 --- a/packages/kit/test/apps/basics/src/routes/load/unchanged/isolated/[slug]/+page.server.js +++ b/packages/kit/test/apps/basics/src/routes/load/unchanged/isolated/[slug]/+page.server.js @@ -1,4 +1,4 @@ -/** @type {import('./$types').PageData} */ +/** @type {import('./$types').PageServerLoad} */ export function load({ params }) { return { slug: params.slug diff --git a/packages/kit/test/apps/basics/src/routes/load/unchanged/uses-parent/[slug]/+page.server.js b/packages/kit/test/apps/basics/src/routes/load/unchanged/uses-parent/[slug]/+page.server.js index 7bc7b8df34d8..fba9a5a3c012 100644 --- a/packages/kit/test/apps/basics/src/routes/load/unchanged/uses-parent/[slug]/+page.server.js +++ b/packages/kit/test/apps/basics/src/routes/load/unchanged/uses-parent/[slug]/+page.server.js @@ -1,4 +1,4 @@ -/** @type {import('./$types').PageData} */ +/** @type {import('./$types').PageServerLoad} */ export async function load({ params, parent }) { const { count } = await parent(); diff --git a/packages/kit/test/apps/basics/test/test.js b/packages/kit/test/apps/basics/test/test.js index 46ec336b5446..c03bcb0aa7ca 100644 --- a/packages/kit/test/apps/basics/test/test.js +++ b/packages/kit/test/apps/basics/test/test.js @@ -876,7 +876,7 @@ test.describe('Load', () => { ).toBe('rgb(255, 0, 0)'); }); - test.only('+layout.server.js does not re-run when downstream load functions are invalidated', async ({ + test('+layout.server.js does not re-run when downstream load functions are invalidated', async ({ page, request, clicknav @@ -892,7 +892,7 @@ test.describe('Load', () => { expect(await page.textContent('h2')).toBe('count: 0'); }); - test.only('+layout.server.js re-runs when await parent() is called from downstream load function', async ({ + test('+layout.server.js re-runs when await parent() is called from downstream load function', async ({ page, request, clicknav From 936d7b36d852c41a1d3c22b3ba9a3d82f382e883 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Fri, 19 Aug 2022 16:50:03 -0400 Subject: [PATCH 05/19] tidy up tests --- .../basics/src/routes/load/unchanged/+layout.svelte | 8 ++++++++ .../src/routes/load/unchanged/reset/+server.js | 9 ++++++++- packages/kit/test/apps/basics/test/test.js | 12 ++++++------ 3 files changed, 22 insertions(+), 7 deletions(-) diff --git a/packages/kit/test/apps/basics/src/routes/load/unchanged/+layout.svelte b/packages/kit/test/apps/basics/src/routes/load/unchanged/+layout.svelte index 2a4c2dcdf06c..59827db10199 100644 --- a/packages/kit/test/apps/basics/src/routes/load/unchanged/+layout.svelte +++ b/packages/kit/test/apps/basics/src/routes/load/unchanged/+layout.svelte @@ -1,2 +1,10 @@ + + diff --git a/packages/kit/test/apps/basics/src/routes/load/unchanged/reset/+server.js b/packages/kit/test/apps/basics/src/routes/load/unchanged/reset/+server.js index 147439045bf7..6ac7a0ccd605 100644 --- a/packages/kit/test/apps/basics/src/routes/load/unchanged/reset/+server.js +++ b/packages/kit/test/apps/basics/src/routes/load/unchanged/reset/+server.js @@ -1,5 +1,12 @@ import { reset } from '../state.js'; -export function POST() { +/** @type {import('./$types').RequestHandler} */ +export function GET({ url }) { reset(); + return new Response(undefined, { + status: 307, + headers: { + location: `${url.origin}/load/unchanged/isolated/a` + } + }); } diff --git a/packages/kit/test/apps/basics/test/test.js b/packages/kit/test/apps/basics/test/test.js index c03bcb0aa7ca..88ac3e97123f 100644 --- a/packages/kit/test/apps/basics/test/test.js +++ b/packages/kit/test/apps/basics/test/test.js @@ -876,12 +876,12 @@ test.describe('Load', () => { ).toBe('rgb(255, 0, 0)'); }); - test('+layout.server.js does not re-run when downstream load functions are invalidated', async ({ + test.only('+layout.server.js does not re-run when downstream load functions are invalidated', async ({ page, request, clicknav }) => { - await request.post('/load/unchanged/reset'); + await request.get('/load/unchanged/reset'); await page.goto('/load/unchanged/isolated/a'); expect(await page.textContent('h1')).toBe('slug: a'); @@ -892,19 +892,19 @@ test.describe('Load', () => { expect(await page.textContent('h2')).toBe('count: 0'); }); - test('+layout.server.js re-runs when await parent() is called from downstream load function', async ({ + test.only('+layout.server.js re-runs when await parent() is called from downstream load function', async ({ page, request, clicknav }) => { - await request.post('/load/unchanged/reset'); + await request.get('/load/unchanged/reset'); - await page.goto('/load/unchanged/with-parent/a'); + await page.goto('/load/unchanged/uses-parent/a'); expect(await page.textContent('h1')).toBe('slug: a'); expect(await page.textContent('h2')).toBe('count: 0'); expect(await page.textContent('h3')).toBe('doubled: 0'); - await clicknav('[href="/load/unchanged/with-parent/b"]'); + await clicknav('[href="/load/unchanged/uses-parent/b"]'); expect(await page.textContent('h1')).toBe('slug: b'); expect(await page.textContent('h2')).toBe('count: 0'); From 9c52730dc8f0d1a89965c027e6d2455dd6bafed9 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Fri, 19 Aug 2022 16:50:13 -0400 Subject: [PATCH 06/19] add Uses type --- packages/kit/types/internal.d.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/kit/types/internal.d.ts b/packages/kit/types/internal.d.ts index b3677941b849..1146c713240b 100644 --- a/packages/kit/types/internal.d.ts +++ b/packages/kit/types/internal.d.ts @@ -292,6 +292,13 @@ export interface SSRState { export type StrictBody = string | Uint8Array; +export interface Uses { + dependencies: Set; + params: Set; + parent: boolean; + url: boolean; +} + export type ValidatedConfig = RecursiveRequired; export type ValidatedKitConfig = RecursiveRequired; From 472dfbbf895848a8ddc8684adc6a49eee35f838b Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Sat, 20 Aug 2022 13:12:24 -0400 Subject: [PATCH 07/19] some stuff i forgot to commit last night, totally forgot what it does, but need to switch branches so im gonna commit it now with an undisciplined commit message --- packages/kit/src/runtime/client/client.js | 242 +++++++++++------- packages/kit/src/runtime/client/types.d.ts | 39 +-- packages/kit/src/runtime/server/index.js | 22 +- .../kit/src/runtime/server/page/load_data.js | 8 +- packages/kit/types/internal.d.ts | 41 ++- 5 files changed, 217 insertions(+), 135 deletions(-) diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index b482a1f06d2d..6f38fc8d0639 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -448,57 +448,50 @@ export function create_client({ target, base, trailing_slash }) { * url: URL; * params: Record; * routeId: string | null; - * server_data: Record | null; + * server_data_node: import('./types').DataNode | null; * }} options * @returns {Promise} */ - async function load_node({ node, parent, url, params, routeId, server_data }) { + async function load_node({ node, parent, url, params, routeId, server_data_node }) { + /** @type {Record | null} */ + let data = null; + + /** @type {import('types').Uses} */ const uses = { - params: new Set(), - url: false, dependencies: new Set(), - parent: false + params: new Set(), + parent: false, + url: false }; - /** @param {string[]} deps */ - function depends(...deps) { - for (const dep of deps) { - const { href } = new URL(dep, url); - uses.dependencies.add(href); + if (node.shared?.load) { + /** @param {string[]} deps */ + function depends(...deps) { + for (const dep of deps) { + const { href } = new URL(dep, url); + uses.dependencies.add(href); + } } - } - /** @type {Record | null} */ - let data = null; - - if (node.server) { - // +page|layout.server.js data means we need to mark this URL as a dependency of itself, - // unless we want to get clever with usage detection on the server, which could - // be returned to the client either as payload or custom headers - uses.dependencies.add(url.href); - uses.url = true; - } - - /** @type {Record} */ - const uses_params = {}; - for (const key in params) { - Object.defineProperty(uses_params, key, { - get() { - uses.params.add(key); - return params[key]; - }, - enumerable: true - }); - } + /** @type {Record} */ + const uses_params = {}; + for (const key in params) { + Object.defineProperty(uses_params, key, { + get() { + uses.params.add(key); + return params[key]; + }, + enumerable: true + }); + } - const load_url = new LoadURL(url); + const load_url = new LoadURL(url); - if (node.shared?.load) { /** @type {import('types').LoadEvent} */ const load_input = { routeId, params: uses_params, - data: server_data ?? null, + data: server_data_node?.data ?? null, get url() { uses.url = true; return load_url; @@ -544,11 +537,9 @@ export function create_client({ target, base, trailing_slash }) { }, setHeaders: () => {}, // noop depends, - get parent() { - // uses.parent assignment here, not on method inokation, else we wouldn't notice when someone - // does await parent() inside an if branch which wasn't executed yet. + parent() { uses.parent = true; - return parent; + return parent(); }, // @ts-expect-error get props() { @@ -583,11 +574,63 @@ export function create_client({ target, base, trailing_slash }) { return { node, - data: data || server_data, - uses + server: server_data_node + ? { + data: server_data_node.data, + uses: { + dependencies: new Set(server_data_node.uses.dependencies ?? []), + params: new Set(server_data_node.uses.params ?? []), + parent: !!server_data_node.uses.parent, + url: !!server_data_node.uses.url + } + } + : null, + shared: node.shared?.load ? { data, uses } : null, + data: data ?? server_data_node?.data ?? null }; } + /** + * @param {import('types').Uses} uses + * @param {boolean} parent_changed + * @param {{ url: boolean, params: string[] }} changed + */ + function detect_change(changed, parent_changed, uses) { + if (uses.parent && parent_changed) return true; + if (changed.url && uses.url) return true; + + for (const param of changed.params) { + if (uses.params.has(param)) return true; + } + + for (const dep of uses.dependencies) { + if (invalidated.some((fn) => fn(dep))) return true; + } + + return false; + } + + /** + * @param {import('types').ServerDataNode | import('types').ServerDataSkippedNode} node + * @param {import('./types').DataNode | null} previous + * @returns {import('./types').DataNode | null} + */ + function create_data_node(node, previous) { + if (node.type === 'data') { + return { + data: node.data, + uses: { + dependencies: new Set(node.uses.dependencies ?? []), + params: new Set(node.uses.params ?? []), + parent: !!node.uses.parent, + url: !!node.uses.url + } + }; + } + + return previous; + } + /** * @param {import('./types').NavigationIntent} intent * @returns {Promise} @@ -609,52 +652,74 @@ export function create_client({ target, base, trailing_slash }) { // to act on the failures at this point) [...errors, ...layouts, leaf].forEach((loader) => loader?.().catch(() => {})); - const nodes = [...layouts, leaf]; + const loaders = [...layouts, leaf]; // To avoid waterfalls when someone awaits a parent, compute as much as possible here already - /** @type {boolean[]} */ - const nodes_changed_since_last_render = []; - for (let i = 0; i < nodes.length; i++) { - if (!nodes[i]) { - nodes_changed_since_last_render.push(false); - } else { - const previous = current.branch[i]; - const changed_since_last_render = - !previous || - (changed.url && previous.uses.url) || - changed.params.some((param) => previous.uses.params.has(param)) || - Array.from(previous.uses.dependencies).some((dep) => invalidated.some((fn) => fn(dep))) || - (previous.uses.parent && nodes_changed_since_last_render.includes(true)); - nodes_changed_since_last_render.push(changed_since_last_render); + + let server_parent_changed = false; + let shared_parent_changed = false; + + /** @type {Array<{ server: boolean, shared: boolean } | null>} */ + const nodes_changed_since_last_render = loaders.map((loader, i) => { + if (!loader) return null; + + const previous = current.branch[i]; + if (!previous) { + return { + server: (server_parent_changed = true), + shared: (shared_parent_changed = true) + }; } - } - /** @type {import('./types').ServerDataPayload | null} */ - let server_data_payload = null; + // TODO if the previous node errored, we might need to always include it? + const { server, shared } = previous; + + const result = { + server: server ? detect_change(changed, server_parent_changed, server.uses) : false, + shared: shared ? detect_change(changed, shared_parent_changed, shared.uses) : false + }; + + if (result.server) server_parent_changed = true; + if (result.shared) shared_parent_changed = true; + + return result; + }); + + console.log(nodes_changed_since_last_render); + + /** @type {import('types').ServerData | null} */ + let server_data = null; if (route.uses_server_data) { try { const res = await native_fetch( - `${url.pathname}${url.pathname.endsWith('/') ? '' : '/'}__data.json${url.search}` + `${url.pathname}${url.pathname.endsWith('/') ? '' : '/'}__data.json${url.search}`, + { + headers: { + 'x-svelte-kit-invalidated': nodes_changed_since_last_render + .map((node) => (node?.server ? '1' : '0')) + .join(',') + } + } ); - server_data_payload = /** @type {import('./types').ServerDataPayload} */ (await res.json()); + server_data = /** @type {import('types').ServerData} */ (await res.json()); if (!res.ok) { - throw server_data_payload; + throw server_data; } } catch (e) { throw new Error('TODO render fallback error page'); } - if (server_data_payload.type === 'redirect') { - return server_data_payload; + if (server_data.type === 'redirect') { + return server_data; } } - const server_data_nodes = server_data_payload?.nodes; + const server_data_nodes = server_data?.nodes; - const branch_promises = nodes.map(async (loader, i) => { + const branch_promises = loaders.map(async (loader, i) => { return Promise.resolve().then(async () => { if (!loader) return; const node = await loader(); @@ -665,14 +730,15 @@ export function create_client({ target, base, trailing_slash }) { nodes_changed_since_last_render[i] || !previous || node !== previous.node; if (changed_since_last_render) { - const payload = server_data_nodes?.[i]; - - if (payload?.httperror) { - throw error(payload.httperror.status, payload.httperror.message); - } - - if (payload?.error) { - throw payload.error; + const server_data_node = server_data_nodes?.[i] ?? null; + + if (server_data_node?.type === 'error') { + if (server_data_node.httperror) { + // reconstruct as an HttpError + throw error(server_data_node.httperror.status, server_data_node.httperror.message); + } else { + throw server_data_node.error; + } } return await load_node({ @@ -687,7 +753,9 @@ export function create_client({ target, base, trailing_slash }) { } return data; }, - server_data: payload?.data ?? null + server_data_node: + server_data_node && + create_data_node(server_data_node, previous ? previous.server : null) }); } else { return previous; @@ -701,8 +769,8 @@ export function create_client({ target, base, trailing_slash }) { /** @type {Array} */ const branch = []; - for (let i = 0; i < nodes.length; i += 1) { - if (nodes[i]) { + for (let i = 0; i < loaders.length; i += 1) { + if (loaders[i]) { try { branch.push(await branch_promises[i]); } catch (e) { @@ -729,12 +797,8 @@ export function create_client({ target, base, trailing_slash }) { error_loaded = { node: await errors[i](), data: {}, - uses: { - params: new Set(), - url: false, - dependencies: new Set(), - parent: false - } + server_uses: null, + shared_uses: null }; return await get_navigation_result_from_branch({ @@ -793,7 +857,8 @@ export function create_client({ target, base, trailing_slash }) { params, routeId, parent: () => Promise.resolve({}), - server_data: null // TODO!!!!! + // TODO!!!!! need to load root layout server data + server_data_node: null }); const root_error = { @@ -1199,12 +1264,13 @@ export function create_client({ target, base, trailing_slash }) { const script = document.querySelector(`script[sveltekit\\:data-type="${type}"]`); return script?.textContent ? JSON.parse(script.textContent) : fallback; }; - const all_server_data = parse('server_data', []); + /** @type {import('types').ServerDataNode[]} */ + const server_data_nodes = parse('server_data', []); const validation_errors = parse('validation_errors', undefined); const branch_promises = node_ids.map(async (n, i) => { // TODO handle case where this contains an "error" or "httperror" - const server_data = all_server_data[i]; + const server_data_node = server_data_nodes[i]; return load_node({ node: await nodes[n](), @@ -1218,7 +1284,7 @@ export function create_client({ target, base, trailing_slash }) { } return data; }, - server_data: server_data?.data ?? null + server_data_node }); }); diff --git a/packages/kit/src/runtime/client/types.d.ts b/packages/kit/src/runtime/client/types.d.ts index 651d6ecd70c2..e047f8f7e455 100644 --- a/packages/kit/src/runtime/client/types.d.ts +++ b/packages/kit/src/runtime/client/types.d.ts @@ -6,7 +6,7 @@ import { prefetch, prefetchRoutes } from '$app/navigation'; -import { CSRPageNode, CSRRoute } from 'types'; +import { CSRPageNode, CSRRoute, Uses } from 'types'; import { HttpError } from '../../index/private.js'; import { SerializedHttpError } from '../server/page/types.js'; @@ -65,15 +65,16 @@ export type NavigationFinished = { export type BranchNode = { node: CSRPageNode; + server: DataNode | null; + shared: DataNode | null; data: Record | null; - uses: { - params: Set; - url: boolean; // TODO make more granular? - dependencies: Set; - parent: boolean; - }; }; +export interface DataNode { + data: Record | null; + uses: Uses; +} + export type NavigationState = { branch: Array; error: HttpError | Error | null; @@ -81,27 +82,3 @@ export type NavigationState = { session_id: number; url: URL; }; - -export type ServerDataPayload = ServerDataRedirected | ServerDataLoaded; - -export interface ServerDataRedirected { - type: 'redirect'; - location: string; -} - -export interface ServerDataLoaded { - type: 'data'; - nodes: Array<{ - data?: Record | null; // TODO or `-1` to indicate 'reuse cached data'? - httperror?: { - status: number; - message: string; - }; - error?: { - name: string; - message: string; - stack: string; - [key: string]: any; - }; - }>; -} diff --git a/packages/kit/src/runtime/server/index.js b/packages/kit/src/runtime/server/index.js index dc843f796df3..162787ee238a 100644 --- a/packages/kit/src/runtime/server/index.js +++ b/packages/kit/src/runtime/server/index.js @@ -258,7 +258,7 @@ export async function respond(request, options, state) { const promises = [...route.layouts, route.leaf].map(async (n, i) => { try { - if (aborted) return; + if (aborted) return null; // == because it could be undefined (in dev) or null (in build, because of JSON.stringify) const node = n == undefined ? n : await options.manifest._.nodes[n](); @@ -270,7 +270,9 @@ export async function respond(request, options, state) { /** @type {Record} */ const data = {}; for (let j = 0; j < i; j += 1) { - const parent = await promises[j]; + const parent = /** @type {import('types').ServerDataNode} */ ( + await promises[j] + ); Object.assign(data, parent.data); } return data; @@ -295,7 +297,7 @@ export async function respond(request, options, state) { length = Math.min(length, i + 1); if (error instanceof HttpError) { - return { httperror: error }; + return { httperror: { ...error } }; } options.handle_error(error, event); @@ -307,18 +309,24 @@ export async function respond(request, options, state) { ) ); - response = json({ + /** @type {import('types').ServerData} */ + const server_data = { type: 'data', nodes: nodes.slice(0, length) - }); + }; + + response = json(server_data); } catch (e) { const error = normalize_error(e); if (error instanceof Redirect) { - response = json({ + /** @type {import('types').ServerData} */ + const server_data = { type: 'redirect', location: error.location - }); + }; + + response = json(server_data); } else { response = json(error_to_pojo(error, options.get_stack), { status: 500 }); } diff --git a/packages/kit/src/runtime/server/page/load_data.js b/packages/kit/src/runtime/server/page/load_data.js index 8d2f378d94d5..c77a6da93a20 100644 --- a/packages/kit/src/runtime/server/page/load_data.js +++ b/packages/kit/src/runtime/server/page/load_data.js @@ -29,12 +29,8 @@ export async function load_server_data({ dev, event, node, parent }) { const params = new Proxy(event.params, { get: (target, key) => { - if (key in target) { - uses.params.add(key); - return target[/** @type {string} */ (key)]; - } - - return undefined; + uses.params.add(key); + return target[/** @type {string} */ (key)]; } }); diff --git a/packages/kit/types/internal.d.ts b/packages/kit/types/internal.d.ts index 1146c713240b..def16b1fc718 100644 --- a/packages/kit/types/internal.d.ts +++ b/packages/kit/types/internal.d.ts @@ -62,9 +62,9 @@ export interface BuildData { export interface CSRPageNode { component: typeof SvelteComponent; shared: { - load: Load; - hydrate: boolean; - router: boolean; + load?: Load; + hydrate?: boolean; + router?: boolean; }; server: boolean; } @@ -169,6 +169,41 @@ export interface Respond { export type RouteData = PageData | EndpointData; +export type ServerData = + | { + type: 'redirect'; + location: string; + } + | { + type: 'data'; + nodes: Array; + }; + +export interface ServerDataNode { + type: 'data'; + data: Record | null; + uses: { + dependencies?: string[]; + params?: string[]; + parent?: number | void; // 1 or undefined + url?: number | void; // 1 or undefined + }; +} + +/** + * Signals that the `load` function was not run, and the + * client should use what it has in memory + */ +export interface ServerDataSkippedNode { + type: 'skip'; +} + +export interface ServerErrorNode { + type: 'error'; + error?: Record; + httperror?: { status: number; message: string }; +} + export interface SSRComponent { default: { render(props: Record): { From 0e691ba134c4d3fec0d39befc18b1d5cade5ee83 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Mon, 22 Aug 2022 15:34:17 -0400 Subject: [PATCH 08/19] some progress --- packages/kit/src/runtime/client/client.js | 209 ++++++++++-------- packages/kit/src/runtime/client/types.d.ts | 5 +- packages/kit/src/runtime/server/index.js | 64 +++--- .../kit/src/runtime/server/page/load_data.js | 1 + packages/kit/src/utils/functions.js | 16 ++ .../routes/load/unchanged/reset/+server.js | 9 +- .../kit/test/apps/basics/test/client.test.js | 38 ++++ packages/kit/test/apps/basics/test/test.js | 36 --- 8 files changed, 218 insertions(+), 160 deletions(-) create mode 100644 packages/kit/src/utils/functions.js diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index 2ad133cb7446..0513a7cfb513 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -16,10 +16,13 @@ const INDEX_KEY = 'sveltekit:index'; const routes = parse(nodes, dictionary, matchers); +const default_layout_loader = nodes[0]; +const default_error_loader = nodes[1]; + // we import the root layout/error nodes eagerly, so that // connectivity errors after initialisation don't nuke the app -const default_layout = nodes[0](); -const default_error = nodes[1](); +default_layout_loader(); +default_error_loader(); // We track the scroll position associated with each history entry in sessionStorage, // rather than on history.state itself, because when navigation is driven by @@ -463,16 +466,24 @@ export function create_client({ target, base, trailing_slash }) { * If `server_data` is passed, this is treated as the initial run and the page endpoint is not requested. * * @param {{ - * node: import('types').CSRPageNode; + * loader: import('types').CSRPageNodeLoader; * parent: () => Promise>; * url: URL; * params: Record; * routeId: string | null; - * server_data_node: import('./types').DataNode | null; + * server_data_node: import('types').ServerDataNode | import('types').ServerErrorNode | null; * }} options * @returns {Promise} */ - async function load_node({ node, parent, url, params, routeId, server_data_node }) { + async function load_node({ loader, parent, url, params, routeId, server_data_node }) { + if (server_data_node?.type === 'error') { + if (server_data_node.httperror) { + throw error(server_data_node.httperror.status, server_data_node.httperror.message); + } + + throw server_data_node.error; + } + /** @type {Record | null} */ let data = null; @@ -484,6 +495,8 @@ export function create_client({ target, base, trailing_slash }) { url: false }; + const node = await loader(); + if (node.shared?.load) { /** @param {string[]} deps */ function depends(...deps) { @@ -605,6 +618,7 @@ export function create_client({ target, base, trailing_slash }) { return { node, + loader, server: server_data_node ? { data: server_data_node.data, @@ -622,11 +636,13 @@ export function create_client({ target, base, trailing_slash }) { } /** - * @param {import('types').Uses} uses + * @param {import('types').Uses | undefined} uses * @param {boolean} parent_changed * @param {{ url: boolean, params: string[] }} changed */ function detect_change(changed, parent_changed, uses) { + if (!uses) return false; + if (uses.parent && parent_changed) return true; if (changed.url && uses.url) return true; @@ -687,49 +703,27 @@ export function create_client({ target, base, trailing_slash }) { // To avoid waterfalls when someone awaits a parent, compute as much as possible here already - let server_parent_changed = false; - let shared_parent_changed = false; + /** @type {import('types').ServerData | null} */ + let server_data = null; - /** @type {Array<{ server: boolean, shared: boolean } | null>} */ - const nodes_changed_since_last_render = loaders.map((loader, i) => { - if (!loader) return null; + const invalid_server_nodes = accumulate(loaders, (loader, i, acc) => { + if (!loader) return false; const previous = current.branch[i]; - if (!previous) { - return { - server: (server_parent_changed = true), - shared: (shared_parent_changed = true) - }; - } - - // TODO if the previous node errored, we might need to always include it? - const { server, shared } = previous; - - const result = { - server: server ? detect_change(changed, server_parent_changed, server.uses) : false, - shared: shared ? detect_change(changed, shared_parent_changed, shared.uses) : false - }; - if (result.server) server_parent_changed = true; - if (result.shared) shared_parent_changed = true; - - return result; + return ( + previous?.loader !== loader || + detect_change(changed, acc.some(Boolean), previous.server?.uses) + ); }); - console.log(nodes_changed_since_last_render); - - /** @type {import('types').ServerData | null} */ - let server_data = null; - - if (route.uses_server_data) { + if (route.uses_server_data && invalid_server_nodes.some(Boolean)) { try { const res = await native_fetch( `${url.pathname}${url.pathname.endsWith('/') ? '' : '/'}__data.json${url.search}`, { headers: { - 'x-svelte-kit-invalidated': nodes_changed_since_last_render - .map((node) => (node?.server ? '1' : '0')) - .join(',') + 'x-svelte-kit-invalidated': invalid_server_nodes.map((x) => (x ? '1' : '')).join(',') } } ); @@ -750,47 +744,49 @@ export function create_client({ target, base, trailing_slash }) { const server_data_nodes = server_data?.nodes; + let parent_changed = false; + const branch_promises = loaders.map(async (loader, i) => { - return Promise.resolve().then(async () => { - if (!loader) return; - const node = await loader(); - - /** @type {import('./types').BranchNode | undefined} */ - const previous = current.branch[i]; - const changed_since_last_render = - nodes_changed_since_last_render[i] || !previous || node !== previous.node; - - if (changed_since_last_render) { - const server_data_node = server_data_nodes?.[i] ?? null; - - if (server_data_node?.type === 'error') { - if (server_data_node.httperror) { - // reconstruct as an HttpError - throw error(server_data_node.httperror.status, server_data_node.httperror.message); - } else { - throw server_data_node.error; - } - } + if (!loader) return; - return await load_node({ - node, - url, - params, - routeId: route.id, - parent: async () => { - const data = {}; - for (let j = 0; j < i; j += 1) { - Object.assign(data, (await branch_promises[j])?.data); - } - return data; - }, - server_data_node: - server_data_node && - create_data_node(server_data_node, previous ? previous.server : null) - }); + /** @type {import('./types').BranchNode | undefined} */ + const previous = current.branch[i]; + + // re-use data from previous load if it's still valid + const valid = + !invalid_server_nodes[i] && + loader === previous?.loader && + !detect_change(changed, parent_changed, previous.shared?.uses); + + if (valid) return previous; + + parent_changed = true; + + const server_data_node = server_data_nodes?.[i] ?? null; + + if (server_data_node?.type === 'error') { + if (server_data_node.httperror) { + // reconstruct as an HttpError + throw error(server_data_node.httperror.status, server_data_node.httperror.message); } else { - return previous; + throw server_data_node.error; } + } + + return load_node({ + loader, + url, + params, + routeId: route.id, + parent: async () => { + const data = {}; + for (let j = 0; j < i; j += 1) { + Object.assign(data, (await branch_promises[j])?.data); + } + return data; + }, + server_data_node: + server_data_node && create_data_node(server_data_node, previous ? previous.server : null) }); }); @@ -827,9 +823,10 @@ export function create_client({ target, base, trailing_slash }) { try { error_loaded = { node: await errors[i](), + loader: errors[i], data: {}, - server_uses: null, - shared_uses: null + server: null, + shared: null }; return await get_navigation_result_from_branch({ @@ -883,7 +880,7 @@ export function create_client({ target, base, trailing_slash }) { const params = {}; // error page does not have params const root_layout = await load_node({ - node: await default_layout, + loader: default_layout_loader, url, params, routeId, @@ -892,16 +889,22 @@ export function create_client({ target, base, trailing_slash }) { server_data_node: null }); + /** @type {import('./types').BranchNode} */ const root_error = { - node: await default_error, - data: null, - // TODO make this unnecessary - uses: { - params: new Set(), - url: false, - dependencies: new Set(), - parent: false - } + node: await default_error_loader(), + loader: default_error_loader, + shared: { + // TODO make all this unnecessary + data: null, + uses: { + params: new Set(), + url: false, + dependencies: new Set(), + parent: false + } + }, + server: null, + data: null }; return await get_navigation_result_from_branch({ @@ -1050,7 +1053,8 @@ export function create_client({ target, base, trailing_slash }) { if (resource === undefined) { // Force rerun of all load functions, regardless of their dependencies for (const node of current.branch) { - node?.uses.dependencies.add(''); + node?.server?.uses.dependencies.add(''); + node?.shared?.uses.dependencies.add(''); } invalidated.push(() => true); } else if (typeof resource === 'function') { @@ -1304,7 +1308,7 @@ export function create_client({ target, base, trailing_slash }) { const server_data_node = server_data_nodes[i]; return load_node({ - node: await nodes[n](), + loader: nodes[n], url, params, routeId, @@ -1315,7 +1319,16 @@ export function create_client({ target, base, trailing_slash }) { } return data; }, - server_data_node + server_data_node: { + data: server_data_node?.data ?? null, + uses: { + // TODO DRY out with create_data_node + dependencies: new Set(server_data_node?.uses.dependencies ?? []), + params: new Set(server_data_node?.uses.params ?? []), + parent: server_data_node?.uses.parent ? true : false, + url: server_data_node?.uses.url ? true : false + } + } }); }); @@ -1356,3 +1369,19 @@ export function create_client({ target, base, trailing_slash }) { } }; } + +/** + * @template T + * @param {T[]} array + * @param {(item: T, i: number, acc: boolean[]) => boolean} fn + */ +function accumulate(array, fn) { + /** @type {boolean[]} */ + const acc = []; + + for (let i = 0; i < array.length; i += 1) { + acc.push(fn(array[i], i, acc)); + } + + return acc; +} diff --git a/packages/kit/src/runtime/client/types.d.ts b/packages/kit/src/runtime/client/types.d.ts index e047f8f7e455..bca855ca10d1 100644 --- a/packages/kit/src/runtime/client/types.d.ts +++ b/packages/kit/src/runtime/client/types.d.ts @@ -6,7 +6,7 @@ import { prefetch, prefetchRoutes } from '$app/navigation'; -import { CSRPageNode, CSRRoute, Uses } from 'types'; +import { CSRPageNode, CSRPageNodeLoader, CSRRoute, ServerDataNode, Uses } from 'types'; import { HttpError } from '../../index/private.js'; import { SerializedHttpError } from '../server/page/types.js'; @@ -65,7 +65,8 @@ export type NavigationFinished = { export type BranchNode = { node: CSRPageNode; - server: DataNode | null; + loader: CSRPageNodeLoader; + server: ServerDataNode | null; shared: DataNode | null; data: Record | null; }; diff --git a/packages/kit/src/runtime/server/index.js b/packages/kit/src/runtime/server/index.js index 162787ee238a..3ad93f11e859 100644 --- a/packages/kit/src/runtime/server/index.js +++ b/packages/kit/src/runtime/server/index.js @@ -10,6 +10,7 @@ import { negotiate } from '../../utils/http.js'; import { HttpError, Redirect } from '../../index/private.js'; import { load_server_data } from './page/load_data.js'; import { json } from '../../index/index.js'; +import { once } from '../../utils/functions.js'; /* global __SVELTEKIT_ADAPTER_NAME__ */ @@ -254,34 +255,47 @@ export async function respond(request, options, state) { let response; if (is_data_request && route.type === 'page') { try { + const node_ids = [...route.layouts, route.leaf]; + + const invalidated = + request.headers.get('x-svelte-kit-invalidated')?.split(',').map(Boolean) ?? + node_ids.map(() => true); + let aborted = false; - const promises = [...route.layouts, route.leaf].map(async (n, i) => { - try { - if (aborted) return null; - - // == because it could be undefined (in dev) or null (in build, because of JSON.stringify) - const node = n == undefined ? n : await options.manifest._.nodes[n](); - return await load_server_data({ - dev: options.dev, - event, - node, - parent: async () => { - /** @type {Record} */ - const data = {}; - for (let j = 0; j < i; j += 1) { - const parent = /** @type {import('types').ServerDataNode} */ ( - await promises[j] - ); - Object.assign(data, parent.data); + const functions = node_ids.map((n, i) => { + return once(async () => { + try { + if (aborted) return null; + + // == because it could be undefined (in dev) or null (in build, because of JSON.stringify) + const node = n == undefined ? n : await options.manifest._.nodes[n](); + return load_server_data({ + dev: options.dev, + event, + node, + parent: async () => { + /** @type {Record} */ + const data = {}; + for (let j = 0; j < i; j += 1) { + const parent = /** @type {import('types').ServerDataNode} */ ( + await functions[j]() + ); + Object.assign(data, parent.data); + } + return data; } - return data; - } - }); - } catch (e) { - aborted = true; - throw e; - } + }); + } catch (e) { + aborted = true; + throw e; + } + }); + }); + + const promises = functions.map(async (fn, i) => { + if (!invalidated[i]) return null; + return fn(); }); let length = promises.length; diff --git a/packages/kit/src/runtime/server/page/load_data.js b/packages/kit/src/runtime/server/page/load_data.js index 9229917f02e0..4ff246ae5d1c 100644 --- a/packages/kit/src/runtime/server/page/load_data.js +++ b/packages/kit/src/runtime/server/page/load_data.js @@ -61,6 +61,7 @@ export async function load_server_data({ dev, event, node, parent }) { } return { + type: 'data', data, uses: { dependencies: uses.dependencies.size > 0 ? Array.from(uses.dependencies) : undefined, diff --git a/packages/kit/src/utils/functions.js b/packages/kit/src/utils/functions.js new file mode 100644 index 000000000000..062910784a41 --- /dev/null +++ b/packages/kit/src/utils/functions.js @@ -0,0 +1,16 @@ +/** + * @template T + * @param {() => T} fn + */ +export function once(fn) { + let done = false; + + /** @type T */ + let result; + + return () => { + if (done) return result; + done = true; + return (result = fn()); + }; +} diff --git a/packages/kit/test/apps/basics/src/routes/load/unchanged/reset/+server.js b/packages/kit/test/apps/basics/src/routes/load/unchanged/reset/+server.js index 6ac7a0ccd605..448545fd5f90 100644 --- a/packages/kit/test/apps/basics/src/routes/load/unchanged/reset/+server.js +++ b/packages/kit/test/apps/basics/src/routes/load/unchanged/reset/+server.js @@ -1,12 +1,7 @@ import { reset } from '../state.js'; /** @type {import('./$types').RequestHandler} */ -export function GET({ url }) { +export function GET() { reset(); - return new Response(undefined, { - status: 307, - headers: { - location: `${url.origin}/load/unchanged/isolated/a` - } - }); + return new Response('ok'); } diff --git a/packages/kit/test/apps/basics/test/client.test.js b/packages/kit/test/apps/basics/test/client.test.js index f95a9568de7c..8e43d8711ef7 100644 --- a/packages/kit/test/apps/basics/test/client.test.js +++ b/packages/kit/test/apps/basics/test/client.test.js @@ -592,3 +592,41 @@ test('can use $app/stores from anywhere on client', async ({ page }) => { await page.click('button'); await expect(page.locator('h1')).toHaveText('/store/client-access'); }); + +test.describe.serial('Invalidation', () => { + test('+layout.server.js does not re-run when downstream load functions are invalidated', async ({ + page, + request, + clicknav + }) => { + await request.get('/load/unchanged/reset'); + + await page.goto('/load/unchanged/isolated/a'); + expect(await page.textContent('h1')).toBe('slug: a'); + expect(await page.textContent('h2')).toBe('count: 0'); + + await clicknav('[href="/load/unchanged/isolated/b"]'); + expect(await page.textContent('h1')).toBe('slug: b'); + expect(await page.textContent('h2')).toBe('count: 0'); + }); + + test('+layout.server.js re-runs when await parent() is called from downstream load function', async ({ + page, + request, + clicknav + }) => { + await request.get('/load/unchanged/reset'); + + await page.goto('/load/unchanged/uses-parent/a'); + expect(await page.textContent('h1')).toBe('slug: a'); + expect(await page.textContent('h2')).toBe('count: 0'); + expect(await page.textContent('h3')).toBe('doubled: 0'); + + await clicknav('[href="/load/unchanged/uses-parent/b"]'); + expect(await page.textContent('h1')).toBe('slug: b'); + expect(await page.textContent('h2')).toBe('count: 0'); + + // this looks wrong, but is actually the intended behaviour (the increment side-effect in a GET would be a bug in a real app) + expect(await page.textContent('h3')).toBe('doubled: 2'); + }); +}); diff --git a/packages/kit/test/apps/basics/test/test.js b/packages/kit/test/apps/basics/test/test.js index 88ac3e97123f..1407da0aeecf 100644 --- a/packages/kit/test/apps/basics/test/test.js +++ b/packages/kit/test/apps/basics/test/test.js @@ -876,42 +876,6 @@ test.describe('Load', () => { ).toBe('rgb(255, 0, 0)'); }); - test.only('+layout.server.js does not re-run when downstream load functions are invalidated', async ({ - page, - request, - clicknav - }) => { - await request.get('/load/unchanged/reset'); - - await page.goto('/load/unchanged/isolated/a'); - expect(await page.textContent('h1')).toBe('slug: a'); - expect(await page.textContent('h2')).toBe('count: 0'); - - await clicknav('[href="/load/unchanged/isolated/b"]'); - expect(await page.textContent('h1')).toBe('slug: b'); - expect(await page.textContent('h2')).toBe('count: 0'); - }); - - test.only('+layout.server.js re-runs when await parent() is called from downstream load function', async ({ - page, - request, - clicknav - }) => { - await request.get('/load/unchanged/reset'); - - await page.goto('/load/unchanged/uses-parent/a'); - expect(await page.textContent('h1')).toBe('slug: a'); - expect(await page.textContent('h2')).toBe('count: 0'); - expect(await page.textContent('h3')).toBe('doubled: 0'); - - await clicknav('[href="/load/unchanged/uses-parent/b"]'); - expect(await page.textContent('h1')).toBe('slug: b'); - expect(await page.textContent('h2')).toBe('count: 0'); - - // this looks wrong, but is actually the intended behaviour (the increment side-effect in a GET would be a bug in a real app) - expect(await page.textContent('h3')).toBe('doubled: 2'); - }); - test('page without load has access to layout data', async ({ page, clicknav }) => { await page.goto('/load/accumulated'); From dc4f089952a932ace69773e2bffe91bc90bbd6b3 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Mon, 22 Aug 2022 16:06:28 -0400 Subject: [PATCH 09/19] fixes --- packages/kit/src/runtime/client/client.js | 4 +++- packages/kit/src/runtime/server/index.js | 15 ++++++++++++--- packages/kit/types/internal.d.ts | 2 +- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index 0513a7cfb513..7e9190240c1e 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -752,9 +752,11 @@ export function create_client({ target, base, trailing_slash }) { /** @type {import('./types').BranchNode | undefined} */ const previous = current.branch[i]; + const can_reuse_server_data = !server_data_nodes?.[i] || server_data_nodes[i].type === 'skip'; + // re-use data from previous load if it's still valid const valid = - !invalid_server_nodes[i] && + can_reuse_server_data && loader === previous?.loader && !detect_change(changed, parent_changed, previous.shared?.uses); diff --git a/packages/kit/src/runtime/server/index.js b/packages/kit/src/runtime/server/index.js index 3ad93f11e859..587d79ed8f70 100644 --- a/packages/kit/src/runtime/server/index.js +++ b/packages/kit/src/runtime/server/index.js @@ -266,7 +266,7 @@ export async function respond(request, options, state) { const functions = node_ids.map((n, i) => { return once(async () => { try { - if (aborted) return null; + if (aborted) return { type: 'skip' }; // == because it could be undefined (in dev) or null (in build, because of JSON.stringify) const node = n == undefined ? n : await options.manifest._.nodes[n](); @@ -294,7 +294,12 @@ export async function respond(request, options, state) { }); const promises = functions.map(async (fn, i) => { - if (!invalidated[i]) return null; + if (!invalidated[i]) { + return { + type: 'skip' + }; + } + return fn(); }); @@ -311,12 +316,16 @@ export async function respond(request, options, state) { length = Math.min(length, i + 1); if (error instanceof HttpError) { - return { httperror: { ...error } }; + return { + type: 'error', + httperror: { ...error } + }; } options.handle_error(error, event); return { + type: 'error', error: error_to_pojo(error, options.get_stack) }; }) diff --git a/packages/kit/types/internal.d.ts b/packages/kit/types/internal.d.ts index def16b1fc718..f790b8fd06e9 100644 --- a/packages/kit/types/internal.d.ts +++ b/packages/kit/types/internal.d.ts @@ -176,7 +176,7 @@ export type ServerData = } | { type: 'data'; - nodes: Array; + nodes: Array; }; export interface ServerDataNode { From 77a89af89d589a45b8601a3365b493bc42e5fc82 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Mon, 22 Aug 2022 16:33:42 -0400 Subject: [PATCH 10/19] types --- packages/kit/src/runtime/client/client.js | 56 +++++-------------- packages/kit/src/runtime/client/types.d.ts | 5 +- packages/kit/src/runtime/server/index.js | 18 +++--- packages/kit/src/runtime/server/page/index.js | 3 +- .../kit/src/runtime/server/page/load_data.js | 10 ++-- packages/kit/types/internal.d.ts | 2 +- 6 files changed, 36 insertions(+), 58 deletions(-) diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index 7e9190240c1e..0323377e554d 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -471,7 +471,7 @@ export function create_client({ target, base, trailing_slash }) { * url: URL; * params: Record; * routeId: string | null; - * server_data_node: import('types').ServerDataNode | import('types').ServerErrorNode | null; + * server_data_node: import('./types').DataNode | import('types').ServerErrorNode | null; * }} options * @returns {Promise} */ @@ -619,18 +619,8 @@ export function create_client({ target, base, trailing_slash }) { return { node, loader, - server: server_data_node - ? { - data: server_data_node.data, - uses: { - dependencies: new Set(server_data_node.uses.dependencies ?? []), - params: new Set(server_data_node.uses.params ?? []), - parent: !!server_data_node.uses.parent, - url: !!server_data_node.uses.url - } - } - : null, - shared: node.shared?.load ? { data, uses } : null, + server: server_data_node, + shared: node.shared?.load ? { type: 'data', data, uses } : null, data: data ?? server_data_node?.data ?? null }; } @@ -658,13 +648,13 @@ export function create_client({ target, base, trailing_slash }) { } /** - * @param {import('types').ServerDataNode | import('types').ServerDataSkippedNode} node - * @param {import('./types').DataNode | null} previous - * @returns {import('./types').DataNode | null} + * @param {import('types').ServerDataNode | import('types').ServerDataSkippedNode | null} node + * @returns {import('./types').DataNode | undefined} */ - function create_data_node(node, previous) { - if (node.type === 'data') { + function create_data_node(node) { + if (node?.type === 'data') { return { + type: 'data', data: node.data, uses: { dependencies: new Set(node.uses.dependencies ?? []), @@ -674,8 +664,6 @@ export function create_client({ target, base, trailing_slash }) { } }; } - - return previous; } /** @@ -752,7 +740,8 @@ export function create_client({ target, base, trailing_slash }) { /** @type {import('./types').BranchNode | undefined} */ const previous = current.branch[i]; - const can_reuse_server_data = !server_data_nodes?.[i] || server_data_nodes[i].type === 'skip'; + const can_reuse_server_data = + !server_data_nodes?.[i] || server_data_nodes[i]?.type === 'skip'; // re-use data from previous load if it's still valid const valid = @@ -787,8 +776,7 @@ export function create_client({ target, base, trailing_slash }) { } return data; }, - server_data_node: - server_data_node && create_data_node(server_data_node, previous ? previous.server : null) + server_data_node: create_data_node(server_data_node) ?? previous?.server ?? null }); }); @@ -895,16 +883,7 @@ export function create_client({ target, base, trailing_slash }) { const root_error = { node: await default_error_loader(), loader: default_error_loader, - shared: { - // TODO make all this unnecessary - data: null, - uses: { - params: new Set(), - url: false, - dependencies: new Set(), - parent: false - } - }, + shared: null, server: null, data: null }; @@ -1321,16 +1300,7 @@ export function create_client({ target, base, trailing_slash }) { } return data; }, - server_data_node: { - data: server_data_node?.data ?? null, - uses: { - // TODO DRY out with create_data_node - dependencies: new Set(server_data_node?.uses.dependencies ?? []), - params: new Set(server_data_node?.uses.params ?? []), - parent: server_data_node?.uses.parent ? true : false, - url: server_data_node?.uses.url ? true : false - } - } + server_data_node: create_data_node(server_data_node) ?? null }); }); diff --git a/packages/kit/src/runtime/client/types.d.ts b/packages/kit/src/runtime/client/types.d.ts index bca855ca10d1..1b32c231ca28 100644 --- a/packages/kit/src/runtime/client/types.d.ts +++ b/packages/kit/src/runtime/client/types.d.ts @@ -6,7 +6,7 @@ import { prefetch, prefetchRoutes } from '$app/navigation'; -import { CSRPageNode, CSRPageNodeLoader, CSRRoute, ServerDataNode, Uses } from 'types'; +import { CSRPageNode, CSRPageNodeLoader, CSRRoute, ServerErrorNode, Uses } from 'types'; import { HttpError } from '../../index/private.js'; import { SerializedHttpError } from '../server/page/types.js'; @@ -66,12 +66,13 @@ export type NavigationFinished = { export type BranchNode = { node: CSRPageNode; loader: CSRPageNodeLoader; - server: ServerDataNode | null; + server: DataNode | null; shared: DataNode | null; data: Record | null; }; export interface DataNode { + type: 'data'; data: Record | null; uses: Uses; } diff --git a/packages/kit/src/runtime/server/index.js b/packages/kit/src/runtime/server/index.js index 587d79ed8f70..02401249f14e 100644 --- a/packages/kit/src/runtime/server/index.js +++ b/packages/kit/src/runtime/server/index.js @@ -266,7 +266,11 @@ export async function respond(request, options, state) { const functions = node_ids.map((n, i) => { return once(async () => { try { - if (aborted) return { type: 'skip' }; + if (aborted) { + return /** @type {import('types').ServerDataSkippedNode} */ ({ + type: 'skip' + }); + } // == because it could be undefined (in dev) or null (in build, because of JSON.stringify) const node = n == undefined ? n : await options.manifest._.nodes[n](); @@ -295,9 +299,9 @@ export async function respond(request, options, state) { const promises = functions.map(async (fn, i) => { if (!invalidated[i]) { - return { + return /** @type {import('types').ServerDataSkippedNode} */ ({ type: 'skip' - }; + }); } return fn(); @@ -316,18 +320,18 @@ export async function respond(request, options, state) { length = Math.min(length, i + 1); if (error instanceof HttpError) { - return { + return /** @type {import('types').ServerErrorNode} */ ({ type: 'error', httperror: { ...error } - }; + }); } options.handle_error(error, event); - return { + return /** @type {import('types').ServerErrorNode} */ ({ type: 'error', error: error_to_pojo(error, options.get_stack) - }; + }); }) ) ); diff --git a/packages/kit/src/runtime/server/page/index.js b/packages/kit/src/runtime/server/page/index.js index 09533cb88be6..d0611692b604 100644 --- a/packages/kit/src/runtime/server/page/index.js +++ b/packages/kit/src/runtime/server/page/index.js @@ -145,6 +145,7 @@ export async function render_page(event, route, options, state, resolve_opts) { /** @type {Error | null} */ let load_error = null; + /** @type {Array>} */ const server_promises = nodes.map((node, i) => { if (load_error) { // if an error happens immediately, don't bother with the rest of the nodes @@ -168,7 +169,7 @@ export async function render_page(event, route, options, state, resolve_opts) { const data = {}; for (let j = 0; j < i; j += 1) { const parent = await server_promises[j]; - Object.assign(data, await parent.data); + if (parent) Object.assign(data, await parent.data); } return data; } diff --git a/packages/kit/src/runtime/server/page/load_data.js b/packages/kit/src/runtime/server/page/load_data.js index 4ff246ae5d1c..2150b5f202d8 100644 --- a/packages/kit/src/runtime/server/page/load_data.js +++ b/packages/kit/src/runtime/server/page/load_data.js @@ -8,6 +8,7 @@ import { LoadURL, PrerenderingURL } from '../../../utils/url.js'; * node: import('types').SSRNode | undefined; * parent: () => Promise>; * }} opts + * @returns {Promise} */ export async function load_server_data({ dev, event, node, parent }) { if (!node?.server) return null; @@ -79,21 +80,22 @@ export async function load_server_data({ dev, event, node, parent }) { * fetcher: typeof fetch; * node: import('types').SSRNode | undefined; * parent: () => Promise>; - * server_data_promise: Promise<{ data: Record } | null>; + * server_data_promise: Promise; * state: import('types').SSRState; * }} opts + * @returns {Promise | null>} */ export async function load_data({ event, fetcher, node, parent, server_data_promise, state }) { - const server_data = await server_data_promise; + const server_data_node = await server_data_promise; if (!node?.shared?.load) { - return server_data?.data; + return server_data_node?.data ?? null; } const load_input = { url: state.prerendering ? new PrerenderingURL(event.url) : new LoadURL(event.url), params: event.params, - data: server_data?.data ?? null, + data: server_data_node?.data ?? null, routeId: event.routeId, fetch: fetcher, setHeaders: event.setHeaders, diff --git a/packages/kit/types/internal.d.ts b/packages/kit/types/internal.d.ts index f790b8fd06e9..def16b1fc718 100644 --- a/packages/kit/types/internal.d.ts +++ b/packages/kit/types/internal.d.ts @@ -176,7 +176,7 @@ export type ServerData = } | { type: 'data'; - nodes: Array; + nodes: Array; }; export interface ServerDataNode { From 3c52cfbe245155ed641fb29a51f2c97dd5277517 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Mon, 22 Aug 2022 16:45:20 -0400 Subject: [PATCH 11/19] fix tests --- packages/kit/test/apps/options/test/test.js | 2 +- packages/kit/test/prerendering/basics/test/test.js | 10 ++++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/kit/test/apps/options/test/test.js b/packages/kit/test/apps/options/test/test.js index dff361ae6b54..629e80d6557f 100644 --- a/packages/kit/test/apps/options/test/test.js +++ b/packages/kit/test/apps/options/test/test.js @@ -161,7 +161,7 @@ test.describe('trailingSlash', () => { expect(r.url()).toBe(`${baseURL}/path-base/page-endpoint/__data.json`); expect(await r.json()).toEqual({ type: 'data', - nodes: [{ data: null }, { data: { message: 'hi' } }] + nodes: [null, { type: 'data', data: { message: 'hi' }, uses: {} }] }); }); diff --git a/packages/kit/test/prerendering/basics/test/test.js b/packages/kit/test/prerendering/basics/test/test.js index e79573f0f333..2c53f006776e 100644 --- a/packages/kit/test/prerendering/basics/test/test.js +++ b/packages/kit/test/prerendering/basics/test/test.js @@ -82,14 +82,20 @@ test('generates __data.json file for shadow endpoints', () => { read('__data.json'), JSON.stringify({ type: 'data', - nodes: [{ data: null }, { data: { message: 'hello' } }] + nodes: [ + { type: 'data', data: null, uses: {} }, + { type: 'data', data: { message: 'hello' }, uses: {} } + ] }) ); assert.equal( read('shadowed-get/__data.json'), JSON.stringify({ type: 'data', - nodes: [{ data: null }, { data: { answer: 42 } }] + nodes: [ + { type: 'data', data: null, uses: {} }, + { type: 'data', data: { answer: 42 }, uses: {} } + ] }) ); }); From a9e50443d6a7b9e683b6671260d5541868b28e5d Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Mon, 22 Aug 2022 16:58:16 -0400 Subject: [PATCH 12/19] gah missed a spot --- packages/kit/test/prerendering/basics/test/test.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/kit/test/prerendering/basics/test/test.js b/packages/kit/test/prerendering/basics/test/test.js index 2c53f006776e..7fe4f813e624 100644 --- a/packages/kit/test/prerendering/basics/test/test.js +++ b/packages/kit/test/prerendering/basics/test/test.js @@ -185,7 +185,10 @@ test('fetches data from local endpoint', () => { read('origin/__data.json'), JSON.stringify({ type: 'data', - nodes: [{ data: null }, { data: { message: 'hello' } }] + nodes: [ + { type: 'data', data: null, uses: {} }, + { type: 'data', data: { message: 'hello' }, uses: null } + ] }) ); assert.equal(read('origin/message.json'), JSON.stringify({ message: 'hello' })); From 11291a3fbdc0d5194575d53fb7cfbe2b54495a70 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Mon, 22 Aug 2022 17:00:37 -0400 Subject: [PATCH 13/19] more fixes --- packages/kit/src/runtime/server/page/index.js | 2 +- .../kit/test/prerendering/basics/test/test.js | 15 +++------------ 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/packages/kit/src/runtime/server/page/index.js b/packages/kit/src/runtime/server/page/index.js index d0611692b604..744af2f2d15c 100644 --- a/packages/kit/src/runtime/server/page/index.js +++ b/packages/kit/src/runtime/server/page/index.js @@ -292,7 +292,7 @@ export async function render_page(event, route, options, state, resolve_opts) { response: new Response(undefined), body: JSON.stringify({ type: 'data', - nodes: branch.map((branch_node) => ({ data: branch_node?.server_data })) + nodes: branch.map((branch_node) => branch_node?.server_data) }) }); } diff --git a/packages/kit/test/prerendering/basics/test/test.js b/packages/kit/test/prerendering/basics/test/test.js index 7fe4f813e624..280a862864a1 100644 --- a/packages/kit/test/prerendering/basics/test/test.js +++ b/packages/kit/test/prerendering/basics/test/test.js @@ -82,20 +82,14 @@ test('generates __data.json file for shadow endpoints', () => { read('__data.json'), JSON.stringify({ type: 'data', - nodes: [ - { type: 'data', data: null, uses: {} }, - { type: 'data', data: { message: 'hello' }, uses: {} } - ] + nodes: [null, { type: 'data', data: { message: 'hello' }, uses: {} }] }) ); assert.equal( read('shadowed-get/__data.json'), JSON.stringify({ type: 'data', - nodes: [ - { type: 'data', data: null, uses: {} }, - { type: 'data', data: { answer: 42 }, uses: {} } - ] + nodes: [null, { type: 'data', data: { answer: 42 }, uses: {} }] }) ); }); @@ -185,10 +179,7 @@ test('fetches data from local endpoint', () => { read('origin/__data.json'), JSON.stringify({ type: 'data', - nodes: [ - { type: 'data', data: null, uses: {} }, - { type: 'data', data: { message: 'hello' }, uses: null } - ] + nodes: [null, { type: 'data', data: { message: 'hello' }, uses: {} }] }) ); assert.equal(read('origin/message.json'), JSON.stringify({ message: 'hello' })); From f9d8fbea6c8ee918bed975ec41650b2684252242 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Mon, 22 Aug 2022 17:40:04 -0400 Subject: [PATCH 14/19] complete a TODO --- packages/kit/src/runtime/client/client.js | 42 ++++++++++++++++++++--- packages/kit/src/runtime/server/index.js | 2 +- 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index 0323377e554d..2cfcceab606a 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -711,7 +711,7 @@ export function create_client({ target, base, trailing_slash }) { `${url.pathname}${url.pathname.endsWith('/') ? '' : '/'}__data.json${url.search}`, { headers: { - 'x-svelte-kit-invalidated': invalid_server_nodes.map((x) => (x ? '1' : '')).join(',') + 'x-sveltekit-invalidated': invalid_server_nodes.map((x) => (x ? '1' : '')).join(',') } } ); @@ -722,7 +722,9 @@ export function create_client({ target, base, trailing_slash }) { throw server_data; } } catch (e) { - throw new Error('TODO render fallback error page'); + // something went catastrophically wrong — bail and defer to the server + native_navigation(url); + return; } if (server_data.type === 'redirect') { @@ -833,6 +835,9 @@ export function create_client({ target, base, trailing_slash }) { } } + // TODO post-https://github.com/sveltejs/kit/discussions/6124, this will + // no longer be necessary — if we get here, it's because the root layout + // load function failed, which means we have to fall back to the server return await load_root_error_page({ status, error, @@ -864,19 +869,48 @@ export function create_client({ target, base, trailing_slash }) { * url: URL; * routeId: string | null * }} opts + * @returns {Promise} */ async function load_root_error_page({ status, error, url, routeId }) { /** @type {Record} */ const params = {}; // error page does not have params + const node = await default_layout_loader(); + + /** @type {import('types').ServerDataNode | null} */ + let server_data_node = null; + + if (node.server) { + // TODO post-https://github.com/sveltejs/kit/discussions/6124 we can use + // existing root layout data + const res = await native_fetch( + `${url.pathname}${url.pathname.endsWith('/') ? '' : '/'}__data.json${url.search}`, + { + headers: { + 'x-sveltekit-invalidated': '1' + } + } + ); + + const server_data_nodes = await res.json(); + server_data_node = server_data_nodes?.[0] ?? null; + + if (!res.ok || server_data_nodes?.type !== 'data') { + // at this point we have no choice but to fall back to the server + native_navigation(url); + + // @ts-expect-error + return; + } + } + const root_layout = await load_node({ loader: default_layout_loader, url, params, routeId, parent: () => Promise.resolve({}), - // TODO!!!!! need to load root layout server data - server_data_node: null + server_data_node: create_data_node(server_data_node) ?? null }); /** @type {import('./types').BranchNode} */ diff --git a/packages/kit/src/runtime/server/index.js b/packages/kit/src/runtime/server/index.js index 02401249f14e..5de561270fd5 100644 --- a/packages/kit/src/runtime/server/index.js +++ b/packages/kit/src/runtime/server/index.js @@ -258,7 +258,7 @@ export async function respond(request, options, state) { const node_ids = [...route.layouts, route.leaf]; const invalidated = - request.headers.get('x-svelte-kit-invalidated')?.split(',').map(Boolean) ?? + request.headers.get('x-sveltekit-invalidated')?.split(',').map(Boolean) ?? node_ids.map(() => true); let aborted = false; From b61c63f302790d2959ab2a19019b4261be2e2085 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Tue, 23 Aug 2022 13:49:29 +0200 Subject: [PATCH 15/19] small tweaks, reduce code --- packages/kit/src/runtime/client/client.js | 43 +++++++---------------- 1 file changed, 12 insertions(+), 31 deletions(-) diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index 2cfcceab606a..7a48e3d32868 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -630,7 +630,7 @@ export function create_client({ target, base, trailing_slash }) { * @param {boolean} parent_changed * @param {{ url: boolean, params: string[] }} changed */ - function detect_change(changed, parent_changed, uses) { + function has_changed(changed, parent_changed, uses) { if (!uses) return false; if (uses.parent && parent_changed) return true; @@ -694,16 +694,16 @@ export function create_client({ target, base, trailing_slash }) { /** @type {import('types').ServerData | null} */ let server_data = null; - const invalid_server_nodes = accumulate(loaders, (loader, i, acc) => { - if (!loader) return false; - + const invalid_server_nodes = loaders.reduce((acc, loader, i) => { const previous = current.branch[i]; + const invalid = + loader && + (previous?.loader !== loader || + has_changed(changed, acc.some(Boolean), previous.server?.uses)); - return ( - previous?.loader !== loader || - detect_change(changed, acc.some(Boolean), previous.server?.uses) - ); - }); + acc.push(invalid); + return acc; + }, /** @type {boolean[]} */ ([])); if (route.uses_server_data && invalid_server_nodes.some(Boolean)) { try { @@ -742,21 +742,18 @@ export function create_client({ target, base, trailing_slash }) { /** @type {import('./types').BranchNode | undefined} */ const previous = current.branch[i]; - const can_reuse_server_data = - !server_data_nodes?.[i] || server_data_nodes[i]?.type === 'skip'; + const server_data_node = server_data_nodes?.[i] ?? null; + const can_reuse_server_data = !server_data_node || server_data_node.type === 'skip'; // re-use data from previous load if it's still valid const valid = can_reuse_server_data && loader === previous?.loader && - !detect_change(changed, parent_changed, previous.shared?.uses); - + !has_changed(changed, parent_changed, previous.shared?.uses); if (valid) return previous; parent_changed = true; - const server_data_node = server_data_nodes?.[i] ?? null; - if (server_data_node?.type === 'error') { if (server_data_node.httperror) { // reconstruct as an HttpError @@ -1375,19 +1372,3 @@ export function create_client({ target, base, trailing_slash }) { } }; } - -/** - * @template T - * @param {T[]} array - * @param {(item: T, i: number, acc: boolean[]) => boolean} fn - */ -function accumulate(array, fn) { - /** @type {boolean[]} */ - const acc = []; - - for (let i = 0; i < array.length; i += 1) { - acc.push(fn(array[i], i, acc)); - } - - return acc; -} From 44a829978a4a1e73f80a3b0ed7cbd1d97d087174 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Tue, 23 Aug 2022 14:31:47 +0200 Subject: [PATCH 16/19] Remove unnecessary code, comment initial server_data gotcha --- packages/kit/src/runtime/client/client.js | 27 +++++++++-------------- packages/kit/src/runtime/server/index.js | 2 +- 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index 7a48e3d32868..4034df4f9f45 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -471,19 +471,11 @@ export function create_client({ target, base, trailing_slash }) { * url: URL; * params: Record; * routeId: string | null; - * server_data_node: import('./types').DataNode | import('types').ServerErrorNode | null; + * server_data_node: import('./types').DataNode | null; * }} options * @returns {Promise} */ async function load_node({ loader, parent, url, params, routeId, server_data_node }) { - if (server_data_node?.type === 'error') { - if (server_data_node.httperror) { - throw error(server_data_node.httperror.status, server_data_node.httperror.message); - } - - throw server_data_node.error; - } - /** @type {Record | null} */ let data = null; @@ -649,7 +641,7 @@ export function create_client({ target, base, trailing_slash }) { /** * @param {import('types').ServerDataNode | import('types').ServerDataSkippedNode | null} node - * @returns {import('./types').DataNode | undefined} + * @returns {import('./types').DataNode | null} */ function create_data_node(node) { if (node?.type === 'data') { @@ -664,6 +656,7 @@ export function create_client({ target, base, trailing_slash }) { } }; } + return null; } /** @@ -907,7 +900,7 @@ export function create_client({ target, base, trailing_slash }) { params, routeId, parent: () => Promise.resolve({}), - server_data_node: create_data_node(server_data_node) ?? null + server_data_node: create_data_node(server_data_node) }); /** @type {import('./types').BranchNode} */ @@ -1311,14 +1304,16 @@ export function create_client({ target, base, trailing_slash }) { const script = document.querySelector(`script[sveltekit\\:data-type="${type}"]`); return script?.textContent ? JSON.parse(script.textContent) : fallback; }; - /** @type {import('types').ServerDataNode[]} */ + /** + * @type {Array} + * On initial navigation, this will only consist of data nodes or `null`. + * A possible error is passed through the `error` property, in which case + * the last entry of `node_ids` is an error page. + */ const server_data_nodes = parse('server_data', []); const validation_errors = parse('validation_errors', undefined); const branch_promises = node_ids.map(async (n, i) => { - // TODO handle case where this contains an "error" or "httperror" - const server_data_node = server_data_nodes[i]; - return load_node({ loader: nodes[n], url, @@ -1331,7 +1326,7 @@ export function create_client({ target, base, trailing_slash }) { } return data; }, - server_data_node: create_data_node(server_data_node) ?? null + server_data_node: create_data_node(server_data_nodes[i]) }); }); diff --git a/packages/kit/src/runtime/server/index.js b/packages/kit/src/runtime/server/index.js index 5de561270fd5..af7e56a84f72 100644 --- a/packages/kit/src/runtime/server/index.js +++ b/packages/kit/src/runtime/server/index.js @@ -317,7 +317,7 @@ export async function respond(request, options, state) { throw error; } - length = Math.min(length, i + 1); + length = i + 1; // don't include nodes after first error if (error instanceof HttpError) { return /** @type {import('types').ServerErrorNode} */ ({ From a945a31593d42f472331e7e5c7ed648bca14e440 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Tue, 23 Aug 2022 14:40:56 +0200 Subject: [PATCH 17/19] comments --- packages/kit/src/runtime/client/client.js | 3 ++- packages/kit/types/internal.d.ts | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index 4034df4f9f45..5a86ff58d570 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -1308,7 +1308,8 @@ export function create_client({ target, base, trailing_slash }) { * @type {Array} * On initial navigation, this will only consist of data nodes or `null`. * A possible error is passed through the `error` property, in which case - * the last entry of `node_ids` is an error page. + * the last entry of `node_ids` is an error page and the last entry of + * `server_data_nodes` is `null`. */ const server_data_nodes = parse('server_data', []); const validation_errors = parse('validation_errors', undefined); diff --git a/packages/kit/types/internal.d.ts b/packages/kit/types/internal.d.ts index def16b1fc718..cfc032bf330c 100644 --- a/packages/kit/types/internal.d.ts +++ b/packages/kit/types/internal.d.ts @@ -200,6 +200,7 @@ export interface ServerDataSkippedNode { export interface ServerErrorNode { type: 'error'; + // Either-or situation, but we don't want to have to do a type assertion error?: Record; httperror?: { status: number; message: string }; } From 9ed88f72f8c019183cc85f8ec0b585ad2d0dfbac Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Tue, 23 Aug 2022 15:00:26 +0200 Subject: [PATCH 18/19] more comments --- packages/kit/types/internal.d.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/kit/types/internal.d.ts b/packages/kit/types/internal.d.ts index cfc032bf330c..2e43e299baff 100644 --- a/packages/kit/types/internal.d.ts +++ b/packages/kit/types/internal.d.ts @@ -179,6 +179,11 @@ export type ServerData = nodes: Array; }; +/** + * Signals a successful response of the server `load` function. + * The `uses` property tells the client when it's possible to reuse this data + * in a subsequent request. + */ export interface ServerDataNode { type: 'data'; data: Record | null; @@ -191,13 +196,16 @@ export interface ServerDataNode { } /** - * Signals that the `load` function was not run, and the + * Signals that the server `load` function was not run, and the * client should use what it has in memory */ export interface ServerDataSkippedNode { type: 'skip'; } +/** + * Signals that the server `load` function failed + */ export interface ServerErrorNode { type: 'error'; // Either-or situation, but we don't want to have to do a type assertion From 79b2a6a8abb081368381656124ef13d390ca7530 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Tue, 23 Aug 2022 16:27:50 +0200 Subject: [PATCH 19/19] changeset --- .changeset/nervous-trees-listen.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/nervous-trees-listen.md diff --git a/.changeset/nervous-trees-listen.md b/.changeset/nervous-trees-listen.md new file mode 100644 index 000000000000..64748db738a9 --- /dev/null +++ b/.changeset/nervous-trees-listen.md @@ -0,0 +1,5 @@ +--- +'@sveltejs/kit': patch +--- + +[feat] Avoid running load on the server unnecessarily