[fix] do not call server and update page.data unnecessarily (#6311)

* [fix] do not call server and update page.data unnecessarily

Fixes #6201
Fixes #6286

* use ~ trick

* remove rerun behavior from root test layout so we can properly test some scenarios

* tabs

* if page doesnt exist yet, data definitely changed

* tweak

Co-authored-by: Rich Harris <hello@rich-harris.dev>

.changeset/tasty-geese-hug.md

@@ -0,0 +1,5 @@
+---
+'@sveltejs/kit': patch
+---
+
+[fix] do not call server and update `$page.data` unnecessarily

packages/kit/src/core/sync/write_client_manifest.js

@@ -48,23 +48,17 @@ export function write_client_manifest(manifest_data, output) {
 			.map((route) => {
 				if (route.page) {
 					const errors = route.page.errors.slice(1).map((n) => n ?? '');
-					const layouts = route.page.layouts.slice(1).map((n) => n ?? '');
+					const layouts = route.page.layouts.slice(1).map((n) => {
+						if (n == undefined) {
+							return '';
+						}
+						return get_node_id(manifest_data.nodes, n);
+					});
 
 					while (layouts.at(-1) === '') layouts.pop();
 					while (errors.at(-1) === '') errors.pop();
 
-					/** @type {import('types').PageNode | null} */
-					let current_node = route.leaf;
-
-					let uses_server_data = false;
-					while (current_node && !uses_server_data) {
-						uses_server_data = !!current_node?.server;
-						current_node = current_node?.parent ?? null;
-					}
-
-					// encode whether or not the route uses the server data
-					// using the ones' complement, to save space
-					const array = [`${uses_server_data ? '~' : ''}${route.page.leaf}`];
+					const array = [get_node_id(manifest_data.nodes, route.page.leaf)];
 
 					// only include non-root layout/error nodes if they exist
 					if (layouts.length > 0 || errors.length > 0) array.push(`[${layouts.join(',')}]`);
@@ -77,6 +71,7 @@ export function write_client_manifest(manifest_data, output) {
 			.join(',\n\t\t')}
 	}`.replace(/^\t/gm, '');
 
+	// String representation of __GENERATED__/client-manifest.js
 	write_if_changed(
 		`${output}/client-manifest.js`,
 		trim(`
@@ -90,3 +85,13 @@ export function write_client_manifest(manifest_data, output) {
 		`)
 	);
 }
+
+/**
+ * Encode whether or not the route uses the server data
+ * using the ones' complement, to save space
+ * @param {import('types').PageNode[]} nodes
+ * @param {number} id
+ */
+function get_node_id(nodes, id) {
+	return `${nodes[id].server ? '~' : ''}${id}`;
+}

packages/kit/src/runtime/client/ambient.d.ts

@@ -8,9 +8,9 @@ declare module '__GENERATED__/client-manifest.js' {
 
 	/**
 	 * A map of `[routeId: string]: [leaf, layouts, errors]` tuples, which
-	 * is parsed into an array of routes on startup. The numbers refer to the
-	 * indices in `nodes`. The route layout and error nodes are not referenced,
-	 * they are always number 0 and 1 and always apply.
+	 * is parsed into an array of routes on startup. The numbers refer to the indices in `nodes`.
+	 * If the number is negative, it means it does use a server load function and the complement is the node index.
+	 * The route layout and error nodes are not referenced, they are always number 0 and 1 and always apply.
 	 */
 	export const dictionary: Record<string, [leaf: number, layouts?: number[], errors?: number[]]>;
 

packages/kit/src/runtime/client/client.js

@@ -424,21 +424,37 @@ export function create_client({ target, base, trailing_slash }) {
 		};
 
 		let data = {};
-		let data_changed = false;
+		let data_changed = !page;
 		for (let i = 0; i < filtered.length; i += 1) {
-			data = { ...data, ...filtered[i].data };
+			const node = filtered[i];
+			data = { ...data, ...node.data };
+
 			// Only set props if the node actually updated. This prevents needless rerenders.
-			if (data_changed || !current.branch.some((node) => node === filtered[i])) {
+			if (data_changed || !current.branch.some((previous) => previous === node)) {
 				result.props[`data_${i}`] = data;
-				data_changed = true;
+				data_changed = data_changed || Object.keys(node.data ?? {}).length > 0;
 			}
 		}
+		if (!data_changed) {
+			// If nothing was added, and the object entries are the same length, this means
+			// that nothing was removed either and therefore the data is the same as the previous one.
+			// This would be more readable with a separate boolean but that would cost us some bytes.
+			data_changed = Object.keys(page.data).length !== Object.keys(data).length;
+		}
 
 		const page_changed =
 			!current.url || url.href !== current.url.href || current.error !== error || data_changed;
 
 		if (page_changed) {
-			result.props.page = { error, params, routeId, status, url, data };
+			result.props.page = {
+				error,
+				params,
+				routeId,
+				status,
+				url,
+				// The whole page store is updated, but this way the object reference stays the same
+				data: data_changed ? data : page.data
+			};
 
 			// TODO remove this for 1.0
 			/**
@@ -675,30 +691,29 @@ export function create_client({ target, base, trailing_slash }) {
 			params: Object.keys(params).filter((key) => current.params[key] !== params[key])
 		};
 
+		const loaders = [...layouts, leaf];
+
 		// 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)
-		[...errors, ...layouts, leaf].forEach((loader) => loader?.().catch(() => {}));
-
-		const loaders = [...layouts, leaf];
-
-		// To avoid waterfalls when someone awaits a parent, compute as much as possible here already
+		errors.forEach((loader) => loader?.().catch(() => {}));
+		loaders.forEach((loader) => loader?.[1]().catch(() => {}));
 
 		/** @type {import('types').ServerData | null} */
 		let server_data = null;
 
 		const invalid_server_nodes = loaders.reduce((acc, loader, i) => {
 			const previous = current.branch[i];
 			const invalid =
-				loader &&
-				(previous?.loader !== loader ||
+				!!loader?.[0] &&
+				(previous?.loader !== loader[1] ||
 					has_changed(changed, acc.some(Boolean), previous.server?.uses));
 
 			acc.push(invalid);
 			return acc;
 		}, /** @type {boolean[]} */ ([]));
 
-		if (route.uses_server_data && invalid_server_nodes.some(Boolean)) {
+		if (invalid_server_nodes.some(Boolean)) {
 			try {
 				const res = await native_fetch(
 					`${url.pathname}${url.pathname.endsWith('/') ? '' : '/'}__data.json${url.search}`,
@@ -741,7 +756,7 @@ export function create_client({ target, base, trailing_slash }) {
 			// re-use data from previous load if it's still valid
 			const valid =
 				can_reuse_server_data &&
-				loader === previous?.loader &&
+				loader[1] === previous?.loader &&
 				!has_changed(changed, parent_changed, previous.shared?.uses);
 			if (valid) return previous;
 
@@ -757,7 +772,7 @@ export function create_client({ target, base, trailing_slash }) {
 			}
 
 			return load_node({
-				loader,
+				loader: loader[1],
 				url,
 				params,
 				routeId: route.id,
@@ -801,11 +816,10 @@ export function create_client({ target, base, trailing_slash }) {
 
 							let j = i;
 							while (!branch[j]) j -= 1;
-
 							try {
 								error_loaded = {
-									node: await errors[i](),
-									loader: errors[i],
+									node: await /** @type {import('types').CSRPageNodeLoader } */ (errors[i])(),
+									loader: /** @type {import('types').CSRPageNodeLoader } */ (errors[i]),
 									data: {},
 									server: null,
 									shared: null
@@ -1087,7 +1101,7 @@ export function create_client({ target, base, trailing_slash }) {
 				: routes;
 
 			const promises = matching.map((r) => {
-				return Promise.all([...r.layouts, r.leaf].map((load) => load?.()));
+				return Promise.all([...r.layouts, r.leaf].map((load) => load?.[1]()));
 			});
 
 			await Promise.all(promises);

packages/kit/src/runtime/client/parse.js

@@ -10,11 +10,6 @@ export function parse(nodes, dictionary, matchers) {
 	return Object.entries(dictionary).map(([id, [leaf, layouts, errors]]) => {
 		const { pattern, names, types } = parse_route_id(id);
 
-		// whether or not the route uses the server data is
-		// encoded using the ones' complement, to save space
-		const uses_server_data = leaf < 0;
-		if (uses_server_data) leaf = ~leaf;
-
 		const route = {
 			id,
 			/** @param {string} path */
@@ -23,9 +18,8 @@ export function parse(nodes, dictionary, matchers) {
 				if (match) return exec(match, names, types, matchers);
 			},
 			errors: [1, ...(errors || [])].map((n) => nodes[n]),
-			layouts: [0, ...(layouts || [])].map((n) => nodes[n]),
-			leaf: nodes[leaf],
-			uses_server_data
+			layouts: [0, ...(layouts || [])].map(create_loader),
+			leaf: create_loader(leaf)
 		};
 
 		// bit of a hack, but ensures that layout/error node lists are the same
@@ -38,4 +32,16 @@ export function parse(nodes, dictionary, matchers) {
 
 		return route;
 	});
+
+	/**
+	 * @param {number} id
+	 * @returns {[boolean, import('types').CSRPageNodeLoader]}
+	 */
+	function create_loader(id) {
+		// whether or not the route uses the server data is
+		// encoded using the ones' complement, to save space
+		const uses_server_data = id < 0;
+		if (uses_server_data) id = ~id;
+		return [uses_server_data, nodes[id]];
+	}
 }

packages/kit/test/apps/basics/src/routes/+layout.js

@@ -1,12 +1,6 @@
-import { error } from '@sveltejs/kit';
-
 /** @type {import('@sveltejs/kit').Load} */
-export async function load({ fetch, url }) {
-	if (url.pathname.startsWith('/errors/error-in-layout')) {
-		const res = await fetch('/errors/error-in-layout/non-existent');
-		throw error(res.status);
-	}
-
+export async function load() {
+	// Do NOT make this load function depend on something which would cause it to rerun
 	return {
 		foo: {
 			bar: 'Custom layout'

packages/kit/test/apps/basics/src/routes/+layout.server.js

@@ -1,4 +1,5 @@
 export async function load() {
+	// Do NOT make this load function depend on something which would cause it to rerun
 	return {
 		rootlayout: 'rootlayout'
 	};

packages/kit/test/apps/basics/src/routes/errors/+layout.js

@@ -0,0 +1,9 @@
+import { error } from '@sveltejs/kit';
+
+/** @type {import('@sveltejs/kit').Load} */
+export async function load({ fetch, url }) {
+	if (url.pathname.startsWith('/errors/error-in-layout')) {
+		const res = await fetch('/errors/error-in-layout/non-existent');
+		throw error(res.status);
+	}
+}

packages/kit/test/apps/basics/src/routes/errors/+layout.svelte

@@ -0,0 +1 @@
+<slot />;

packages/kit/test/apps/basics/src/routes/store/data/unchanged/+layout.svelte

@@ -0,0 +1,14 @@
+<script>
+	import { page } from '$app/stores';
+	let previous = $page.data;
+	let count = 0;
+	$: {
+		if (previous !== $page.data) {
+			count++;
+		}
+	}
+</script>
+
+<p>$page.data was updated {count} time(s)</p>
+<a href="/store/data/unchanged/a">a</a>
+<a href="/store/data/unchanged/b">b</a>

packages/kit/test/apps/basics/src/routes/store/data/unchanged/a/+page.svelte

@@ -0,0 +1 @@
+Page A

packages/kit/test/apps/basics/src/routes/store/data/unchanged/b/+page.svelte

@@ -0,0 +1 @@
+Page B

packages/kit/test/apps/basics/test/client.test.js

@@ -598,11 +598,19 @@ test('Can use browser-only global on client-only page', async ({ page, read_erro
 	expect(read_errors('/no-ssr/browser-only-global')).toBe(undefined);
 });
 
-test('can use $app/stores from anywhere on client', async ({ page }) => {
-	await page.goto('/store/client-access');
-	await expect(page.locator('h1')).toHaveText('undefined');
-	await page.click('button');
-	await expect(page.locator('h1')).toHaveText('/store/client-access');
+test.describe('$app/stores', () => {
+	test('can use $app/stores from anywhere on client', async ({ page }) => {
+		await page.goto('/store/client-access');
+		await expect(page.locator('h1')).toHaveText('undefined');
+		await page.click('button');
+		await expect(page.locator('h1')).toHaveText('/store/client-access');
+	});
+
+	test('$page.data does not update if data is unchanged', async ({ page, app }) => {
+		await page.goto('/store/data/unchanged/a');
+		await app.goto('/store/data/unchanged/b');
+		await expect(page.locator('p')).toHaveText('$page.data was updated 0 time(s)');
+	});
 });
 
 test.describe.serial('Invalidation', () => {

packages/kit/types/internal.d.ts

@@ -71,13 +71,16 @@ export interface CSRPageNode {
 
 export type CSRPageNodeLoader = () => Promise<CSRPageNode>;
 
+/**
+ * Definition of a client side route.
+ * The boolean in the tuples indicates whether the route has a server load.
+ */
 export type CSRRoute = {
 	id: string;
 	exec: (path: string) => undefined | Record<string, string>;
-	errors: CSRPageNodeLoader[];
-	layouts: CSRPageNodeLoader[];
-	leaf: CSRPageNodeLoader;
-	uses_server_data: boolean;
+	errors: Array<CSRPageNodeLoader | undefined>;
+	layouts: Array<[boolean, CSRPageNodeLoader] | undefined>;
+	leaf: [boolean, CSRPageNodeLoader];
 };
 
 export type GetParams = (match: RegExpExecArray) => Record<string, string>;