fix: refresh layout types when a page module changes

.changeset/refresh-layout-types.md

@@ -0,0 +1,5 @@
+---
+'@sveltejs/kit': patch
+---
+
+fix: refresh layout types when a page gains or loses a `load` function

packages/kit/src/core/sync/write_types/index.js

@@ -15,17 +15,16 @@ const is_whitespace = (/** @type {string} */ char) => /\s/.test(char);
  *   file_name: string;
  *   modified: boolean;
  *   code: string;
- *   exports: any[];
+ *   exports: string[];
  *  } | null} Proxy
  *
- *  @typedef {{
- *   server: Proxy,
- *   universal: Proxy
- *  }} Proxies
- *
- *  @typedef {Map<import('types').PageNode, {route: import('types').RouteData, proxies: Proxies}>} RoutesMap
+ *  @typedef {Map<import('types').PageNode, import('types').RouteData>} RoutesMap
  */
 
+// proxies are reused across syncs until the module they were made from changes
+/** @type {Map<string, { content: string, proxy: Proxy }>} */
+const proxy_cache = new Map();
+
 /**
  * Creates types for the whole manifest
  * @param {import('types').ValidatedConfig} config
@@ -80,7 +79,23 @@ export function write_types(config, manifest_data, file, root) {
 	if (!route) return;
 	if (!route.leaf && !route.layout && !route.endpoint) return; // nothing to do
 
-	update_types(config, create_routes_map(manifest_data), route, root);
+	// a layout's types depend on the `load` exports of every page below it
+	const page_module = [route.leaf?.universal, route.leaf?.server].find(
+		(f) => f && path.resolve(root, f) === path.resolve(file)
+	);
+	const declares_load = (/** @type {string} */ f) =>
+		proxy_cache.get(f)?.proxy?.exports.includes('load');
+	const declared_load = page_module && declares_load(page_module);
+
+	const routes_map = create_routes_map(manifest_data);
+	update_types(config, routes_map, route, root);
+
+	if (!page_module || declares_load(page_module) === declared_load) return;
+
+	for (let node = route.leaf?.parent; node; node = node.parent) {
+		const layout_route = manifest_data.routes.find((r) => r.layout === node);
+		if (layout_route) update_types(config, routes_map, layout_route, root);
+	}
 }
 
 /**
@@ -91,9 +106,7 @@ function create_routes_map(manifest_data) {
 	/** @type {RoutesMap} */
 	const map = new Map();
 	for (const route of manifest_data.routes) {
-		if (route.leaf) {
-			map.set(route.leaf, { route, proxies: { server: null, universal: null } });
-		}
+		if (route.leaf) map.set(route.leaf, route);
 	}
 	return map;
 }
@@ -188,24 +201,11 @@ function update_types(config, routes, route, root) {
 	}
 
 	if (route.leaf) {
-		let route_info = routes.get(route.leaf);
-		if (!route_info) {
-			// This should be defined, but belts and braces
-			route_info = { route, proxies: { server: null, universal: null } };
-			routes.set(route.leaf, route_info);
-		}
-
-		const { declarations: d, exports: e } = process_node(
-			route.leaf,
-			outdir,
-			true,
-			route_info.proxies,
-			root
-		);
+		const { declarations: d, exports: e, proxies } = process_node(route.leaf, outdir, true, root);
 
 		exports.push(...e);
 		declarations.push(...d);
-		write_proxies(route_info.proxies);
+		write_proxies(proxies);
 
 		if (route.leaf.server) {
 			exports.push(
@@ -229,26 +229,25 @@ function update_types(config, routes, route, root) {
 		route.layout.child_pages?.forEach((page) => {
 			const leaf = routes.get(page);
 			if (leaf) {
-				if (leaf.route.page) ids.push(`"${leaf.route.id}"`);
+				if (leaf.page) ids.push(`"${leaf.id}"`);
 
-				for (const param of leaf.route.params) {
+				for (const param of leaf.params) {
 					// skip if already added
 					if (layout_params.some((p) => p.name === param.name)) continue;
 					layout_params.push({ ...param, optional: true });
 				}
+			}
 
-				ensureProxies(page, leaf.proxies, root);
+			const server = page.server ? get_proxy(page.server, true, root) : null;
+			const universal = page.universal ? get_proxy(page.universal, false, root) : null;
 
-				if (
-					// Be defensive - if a proxy doesn't exist (because it couldn't be created), assume a load function exists.
-					// If we didn't and it's a false negative, the user could wrongfully get a type error on layouts.
-					(leaf.proxies.server && !leaf.proxies.server.exports.includes('load')) ||
-					(leaf.proxies.universal && !leaf.proxies.universal.exports.includes('load'))
-				) {
-					all_pages_have_load = false;
-				}
-			}
-			if (!page.server && !page.universal) {
+			if (
+				(!page.server && !page.universal) ||
+				// Be defensive - if a proxy doesn't exist (because it couldn't be created), assume a load function exists.
+				// If we didn't and it's a false negative, the user could wrongfully get a type error on layouts.
+				(server && !server.exports.includes('load')) ||
+				(universal && !universal.exports.includes('load'))
+			) {
 				all_pages_have_load = false;
 			}
 		});
@@ -264,16 +263,11 @@ function update_types(config, routes, route, root) {
 			'type LayoutParams = RouteParams & ' + generate_params_type(layout_params, outdir, config)
 		);
 
-		/** @type {Proxies} */
-		const proxies = { server: null, universal: null };
-		const { exports: e, declarations: d } = process_node(
-			route.layout,
-			outdir,
-			false,
-			proxies,
-			root,
-			all_pages_have_load
-		);
+		const {
+			exports: e,
+			declarations: d,
+			proxies
+		} = process_node(route.layout, outdir, false, root, all_pages_have_load);
 
 		exports.push(...e);
 		declarations.push(...d);
@@ -305,9 +299,9 @@ function update_types(config, routes, route, root) {
 		}
 	}
 
-	/** @param {Proxies} proxies */
+	/** @param {Proxy[]} proxies */
 	function write_proxies(proxies) {
-		for (const proxy of [proxies.server, proxies.universal]) {
+		for (const proxy of proxies) {
 			if (proxy?.modified) write_file(proxy.file_name, proxy.code);
 		}
 	}
@@ -317,11 +311,10 @@ function update_types(config, routes, route, root) {
  * @param {import('types').PageNode} node
  * @param {string} outdir
  * @param {boolean} is_page
- * @param {Proxies} proxies
  * @param {string} root The project root directory
  * @param {boolean} [all_pages_have_load]
  */
-function process_node(node, outdir, is_page, proxies, root, all_pages_have_load = true) {
+function process_node(node, outdir, is_page, root, all_pages_have_load = true) {
 	const params = `${is_page ? 'Route' : 'Layout'}Params`;
 	const prefix = is_page ? 'Page' : 'Layout';
 
@@ -337,11 +330,13 @@ function process_node(node, outdir, is_page, proxies, root, all_pages_have_load
 	/** @type {string} */
 	let data;
 
-	ensureProxies(node, proxies, root);
+	/** @type {Proxy[]} */
+	const proxies = [];
 
 	if (node.server) {
 		const basename = path.basename(node.server);
-		const proxy = proxies.server;
+		const proxy = get_proxy(node.server, true, root);
+		proxies.push(proxy);
 
 		server_data = get_data_type(node.server, 'null', proxy, true);
 
@@ -391,7 +386,8 @@ function process_node(node, outdir, is_page, proxies, root, all_pages_have_load
 	declarations.push(`type ${parent_type} = ${get_parent_type(node, 'LayoutData')};`);
 
 	if (node.universal) {
-		const proxy = proxies.universal;
+		const proxy = get_proxy(node.universal, false, root);
+		proxies.push(proxy);
 
 		const type = get_data_type(
 			node.universal,
@@ -418,7 +414,7 @@ function process_node(node, outdir, is_page, proxies, root, all_pages_have_load
 
 	exports.push(`export type ${prefix}Data = ${data};`);
 
-	return { declarations, exports };
+	return { declarations, exports, proxies };
 
 	/**
 	 * @param {string} file_path
@@ -446,41 +442,26 @@ function process_node(node, outdir, is_page, proxies, root, all_pages_have_load
 }
 
 /**
- * This function populates the proxies object, if necessary and not already done.
  * Proxies are used to tweak the code of a file before it's typechecked.
- * They are needed in two places - when generating the types for a page or layout.
- * To not do the same work twice, we generate the proxies once and pass them around.
- *
- * @param {import('types').PageNode} node
- * @param {Proxies} proxies
- * @param {string} root The project root directory
- */
-function ensureProxies(node, proxies, root) {
-	if (node.server && !proxies.server) {
-		proxies.server = createProxy(node.server, true, root);
-	}
-
-	if (node.universal && !proxies.universal) {
-		proxies.universal = createProxy(node.universal, false, root);
-	}
-}
-
-/**
  * @param {string} file_path
  * @param {boolean} is_server
  * @param {string} root The project root directory
  * @returns {Proxy}
  */
-function createProxy(file_path, is_server, root) {
-	const proxy = tweak_types(fs.readFileSync(path.resolve(root, file_path), 'utf8'), is_server);
-	if (proxy) {
-		return {
-			...proxy,
-			file_name: `proxy${path.basename(file_path)}`
+function get_proxy(file_path, is_server, root) {
+	const content = fs.readFileSync(path.resolve(root, file_path), 'utf8');
+	let cached = proxy_cache.get(file_path);
+
+	if (cached?.content !== content) {
+		const proxy = tweak_types(content, is_server);
+		cached = {
+			content,
+			proxy: proxy && { ...proxy, file_name: `proxy${path.basename(file_path)}` }
 		};
-	} else {
-		return null;
+		proxy_cache.set(file_path, cached);
 	}
+
+	return cached.proxy;
 }
 
 /**

packages/kit/src/core/sync/write_types/index.spec.js

@@ -1,10 +1,11 @@
 import { execSync } from 'node:child_process';
 import fs from 'node:fs';
+import os from 'node:os';
 import path from 'node:path';
 import process from 'node:process';
 import { assert, describe, expect, test } from 'vitest';
 import create_manifest_data from '../create_manifest_data/index.js';
-import { tweak_types, write_all_types } from './index.js';
+import { tweak_types, write_all_types, write_types } from './index.js';
 import { write_app_types } from '../write_app_types.js';
 import { validate_config } from '../../config/index.js';
 import { write_env } from '../write_env.js';
@@ -54,6 +55,53 @@ test('removes types of deleted routes and proxies that are no longer written', (
 	expect(fs.existsSync(path.join(types, 'sub/$types.d.ts'))).toBe(false);
 });
 
+test('refreshes layout types when a page gains or loses a load function', () => {
+	const root = fs.mkdtempSync(path.join(os.tmpdir(), 'write-types-'));
+	const routes = path.join(root, 'src/routes');
+	fs.mkdirSync(path.join(routes, 'sub'), { recursive: true });
+	fs.writeFileSync(path.join(routes, '+layout.server.js'), 'export function load() {}');
+	fs.writeFileSync(path.join(routes, '+page.svelte'), '');
+	fs.writeFileSync(path.join(routes, '+page.js'), 'export function load() {}');
+	fs.writeFileSync(path.join(routes, 'sub/+page.svelte'), '');
+	fs.writeFileSync(path.join(routes, 'sub/+page.js'), '');
+
+	const config = validate_config({});
+	config.files.assets = path.join(root, 'static');
+	config.files.params = path.join(root, 'src/params');
+	config.files.routes = routes;
+	config.outDir = path.join(root, '.svelte-kit');
+	const manifest = create_manifest_data(config, root);
+	const layout_types = path.join(config.outDir, 'types/src/routes/$types.d.ts');
+	const shape = () =>
+		fs.readFileSync(layout_types, 'utf-8').includes('LayoutServerLoad<OutputData extends Partial');
+
+	const page = path.join(routes, 'sub/+page.js');
+	const marker = '// untouched';
+	/** @param {string} code */
+	const save = (code) => {
+		fs.appendFileSync(layout_types, marker);
+		fs.writeFileSync(page, code);
+		write_types(config, manifest, page, root);
+		return !fs.readFileSync(layout_types, 'utf-8').endsWith(marker);
+	};
+
+	try {
+		write_all_types(config, manifest, root);
+		expect(shape()).toBe(false);
+
+		expect(save('export function load() {}')).toBe(true);
+		expect(shape()).toBe(true);
+
+		// same `load` presence, layouts are left alone
+		expect(save('export function load() { return {}; }')).toBe(false);
+
+		expect(save('')).toBe(true);
+		expect(shape()).toBe(false);
+	} finally {
+		fs.rmSync(root, { recursive: true, force: true });
+	}
+});
+
 describe('Creates correct $types', () => {
 	// To save us from creating a real SvelteKit project for each of the tests,
 	// we first run the type generation directly for each test case, and then