diff --git a/.changeset/big-eels-watch.md b/.changeset/big-eels-watch.md new file mode 100644 index 000000000000..f2eed653a812 --- /dev/null +++ b/.changeset/big-eels-watch.md @@ -0,0 +1,5 @@ +--- +'@sveltejs/kit': patch +--- + +fix: correctly invalidate static analysis cache of child nodes when modifying a universal `+layout` file during dev diff --git a/packages/kit/src/core/postbuild/analyse.js b/packages/kit/src/core/postbuild/analyse.js index 713f4ea66141..ba8a013b32f0 100644 --- a/packages/kit/src/core/postbuild/analyse.js +++ b/packages/kit/src/core/postbuild/analyse.js @@ -63,6 +63,7 @@ async function analyse({ internal.set_manifest(manifest); internal.set_read_implementation((file) => createReadableStream(`${server_root}/server/${file}`)); + /** @type {Map | null, children: string[] }>} */ const static_exports = new Map(); // first, build server nodes without the client manifest so we can analyse it diff --git a/packages/kit/src/exports/vite/build/build_server.js b/packages/kit/src/exports/vite/build/build_server.js index 9332322aa572..20f7bf683842 100644 --- a/packages/kit/src/exports/vite/build/build_server.js +++ b/packages/kit/src/exports/vite/build/build_server.js @@ -15,7 +15,7 @@ import { create_node_analyser } from '../static_analysis/index.js'; * @param {import('vite').Manifest | null} client_manifest * @param {import('vite').Rollup.OutputAsset[] | null} css * @param {import('types').RecursiveRequired} output_config - * @param {Map | null>} static_exports + * @param {Map | null, children: string[] }>} static_exports */ export async function build_server_nodes(out, kit, manifest_data, server_manifest, client_manifest, css, output_config, static_exports) { mkdirp(`${out}/server/nodes`); diff --git a/packages/kit/src/exports/vite/dev/index.js b/packages/kit/src/exports/vite/dev/index.js index 28beac7603b2..818cc09096ef 100644 --- a/packages/kit/src/exports/vite/dev/index.js +++ b/packages/kit/src/exports/vite/dev/index.js @@ -355,14 +355,18 @@ export async function dev(vite, vite_config, svelte_config) { // Debounce add/unlink events because in case of folder deletion or moves // they fire in rapid succession, causing needless invocations. + // These watchers only run for routes, param matchers, and client hooks. watch('add', () => debounce(update_manifest)); watch('unlink', () => debounce(update_manifest)); watch('change', (file) => { // Don't run for a single file if the whole manifest is about to get updated if (timeout || restarting) return; + if (/\+(page|layout).*$/.test(file)) { + invalidate_page_options(path.relative(cwd, file)); + } + sync.update(svelte_config, manifest_data, file); - invalidate_page_options(path.relative(cwd, file)); }); const { appTemplate, errorTemplate, serviceWorker, hooks } = svelte_config.kit.files; @@ -389,10 +393,10 @@ export async function dev(vite, vite_config, svelte_config) { } }); - // changing the svelte config requires restarting the dev server - // the config is only read on start and passed on to vite-plugin-svelte - // which needs up-to-date values to operate correctly vite.watcher.on('change', async (file) => { + // changing the svelte config requires restarting the dev server + // the config is only read on start and passed on to vite-plugin-svelte + // which needs up-to-date values to operate correctly if (path.basename(file) === 'svelte.config.js') { console.log(`svelte config changed, restarting vite dev-server. changed file: ${file}`); restarting = true; diff --git a/packages/kit/src/exports/vite/static_analysis/index.js b/packages/kit/src/exports/vite/static_analysis/index.js index 94600d19e574..e6e56df7b8e9 100644 --- a/packages/kit/src/exports/vite/static_analysis/index.js +++ b/packages/kit/src/exports/vite/static_analysis/index.js @@ -4,23 +4,22 @@ import { read } from '../../../utils/filesystem.js'; const inheritable_page_options = new Set(['ssr', 'prerender', 'csr', 'trailingSlash', 'config']); -const page_options = new Set([...inheritable_page_options, 'entries']); +const valid_page_options = new Set([...inheritable_page_options, 'entries']); const skip_parsing_regex = new RegExp( - `${Array.from(page_options).join('|')}|(?:export[\\s\\n]+\\*[\\s\\n]+from)` + `${Array.from(valid_page_options).join('|')}|(?:export[\\s\\n]+\\*[\\s\\n]+from)` ); const parser = Parser.extend(tsPlugin()); /** - * Collects exported page options from a +page.js/+layout.js file. - * We ignore reassignments and use the declared value. - * Returns `null` if any export is too difficult to analyse. - * @param {string} filename + * Collects page options from a +page.js/+layout.js file, ignoring reassignments + * and using the declared value. Returns `null` if any export is too difficult to analyse. + * @param {string} filename The name of the file to report when an error occurs * @param {string} input * @returns {Record | null} */ -export function statically_analyse_exports(filename, input) { +export function statically_analyse_page_options(filename, input) { // if there's a chance there are no page exports or export all declaration, // then we can skip the AST parsing which is expensive if (!skip_parsing_regex.test(input)) { @@ -34,14 +33,14 @@ export function statically_analyse_exports(filename, input) { }); /** @type {Map} */ - const static_exports = new Map(); + const page_options = new Map(); for (const statement of source.body) { // ignore export all declarations with aliases that are not page options if ( statement.type === 'ExportAllDeclaration' && statement.exported && - !page_options.has(get_name(statement.exported)) + !valid_page_options.has(get_name(statement.exported)) ) { continue; } @@ -60,7 +59,7 @@ export function statically_analyse_exports(filename, input) { const export_specifiers = new Map(); for (const specifier of statement.specifiers) { const exported_name = get_name(specifier.exported); - if (!page_options.has(exported_name)) { + if (!valid_page_options.has(exported_name)) { continue; } @@ -109,7 +108,7 @@ export function statically_analyse_exports(filename, input) { } if (variable_declarator.init?.type === 'Literal') { - static_exports.set( + page_options.set( /** @type {string} */ (export_specifiers.get(variable_declarator.id.name)), variable_declarator.init.value ); @@ -138,7 +137,7 @@ export function statically_analyse_exports(filename, input) { // class and function declarations if (statement.declaration.type !== 'VariableDeclaration') { - if (page_options.has(statement.declaration.id.name)) { + if (valid_page_options.has(statement.declaration.id.name)) { return null; } continue; @@ -149,12 +148,12 @@ export function statically_analyse_exports(filename, input) { return null; } - if (!page_options.has(declaration.id.name)) { + if (!valid_page_options.has(declaration.id.name)) { continue; } if (declaration.init?.type === 'Literal') { - static_exports.set(declaration.id.name, declaration.init.value); + page_options.set(declaration.id.name, declaration.init.value); continue; } @@ -163,10 +162,10 @@ export function statically_analyse_exports(filename, input) { } } - return Object.fromEntries(static_exports); + return Object.fromEntries(page_options); } catch (error) { if (error instanceof Error) { - error.message = `Failed to statically analyse ${filename}. ${error.message}`; + error.message = `Failed to statically analyse page options for ${filename}. ${error.message}`; } throw error; } @@ -183,65 +182,67 @@ export function get_name(node) { /** * @param {{ * resolve: (file: string) => Promise>; - * static_exports?: Map | null>; + * static_exports?: Map | null, children: string[] }>; * }} opts */ export function create_node_analyser({ resolve, static_exports = new Map() }) { /** * Computes the final page options for a node (if possible). Otherwise, returns `null`. * @param {import('types').PageNode} node - * @returns {Promise} + * @returns {Promise | null>} */ const get_page_options = async (node) => { - if (node.universal && static_exports.has(node.universal)) { - return /** @type {import('types').UniversalNode | null} */ ( - static_exports.get(node.universal) - ); + const key = node.universal || node.server; + if (key && static_exports.has(key)) { + return { ...static_exports.get(key)?.page_options }; } - /** @type {Record | null} */ + /** @type {Record} */ let page_options = {}; - if (node.server) { - const module = await resolve(node.server); - for (const key in inheritable_page_options) { - if (key in module) { - page_options[key] = module[key]; - } - } - } + if (node.parent) { + const parent_options = await get_page_options(node.parent); - if (node.universal) { - let universal_exports = static_exports.get(node.universal); - if (universal_exports === undefined) { - const input = read(node.universal); - universal_exports = statically_analyse_exports(node.universal, input); + const parent_key = node.parent.universal || node.parent.server; + if (key && parent_key) { + static_exports.get(parent_key)?.children.push(key); } - if (universal_exports === null) { - static_exports.set(node.universal, null); + if (parent_options === null) { + // if the parent cannot be analysed, we can't know what page options + // the child node inherits, so we also mark it as unanalysable + if (key) { + static_exports.set(key, { page_options: null, children: [] }); + } return null; } - page_options = { ...page_options, ...universal_exports }; + page_options = { ...parent_options }; } - if (node.parent) { - const parent_options = await get_page_options(node.parent); - if (parent_options === null) { - // if the parent cannot be statically analysed, we can't know what - // page options the current node inherits, so we invalidate it too - if (node.universal) { - static_exports.set(node.universal, null); + if (node.server) { + const module = await resolve(node.server); + for (const page_option in inheritable_page_options) { + if (page_option in module) { + page_options[page_option] = module[page_option]; } + } + } + + if (node.universal) { + const input = read(node.universal); + const universal_page_options = statically_analyse_page_options(node.universal, input); + + if (universal_page_options === null) { + static_exports.set(node.universal, { page_options: null, children: [] }); return null; } - page_options = { ...parent_options, ...page_options }; + page_options = { ...page_options, ...universal_page_options }; } - if (node.universal) { - static_exports.set(node.universal, page_options); + if (key) { + static_exports.set(key, { page_options, children: [] }); } return page_options; @@ -249,11 +250,14 @@ export function create_node_analyser({ resolve, static_exports = new Map() }) { /** * @param {string} file - * @returns {void} */ const invalidate_page_options = (file) => { + static_exports.get(file)?.children.forEach((child) => static_exports.delete(child)); static_exports.delete(file); }; - return { get_page_options, invalidate_page_options }; + return { + get_page_options, + invalidate_page_options + }; } diff --git a/packages/kit/src/exports/vite/static_analysis/index.spec.js b/packages/kit/src/exports/vite/static_analysis/index.spec.js index 2514555d3033..6c94431b6fb6 100644 --- a/packages/kit/src/exports/vite/static_analysis/index.spec.js +++ b/packages/kit/src/exports/vite/static_analysis/index.spec.js @@ -1,5 +1,5 @@ import { expect, test } from 'vitest'; -import { statically_analyse_exports } from './index.js'; +import { statically_analyse_page_options } from './index.js'; test.each([ [ @@ -18,7 +18,7 @@ test.each([ ` ] ])('page option is assigned a literal value: %s', (_, input) => { - const exports = statically_analyse_exports('', input); + const exports = statically_analyse_page_options('', input); expect(exports).toEqual({ ssr: false, csr: true, prerender: 'auto', trailingSlash: 'always' }); }); @@ -52,7 +52,7 @@ test.each([ ], ['export all declaration alias', "export * as ssr from './foo'"] ])('fails when page option is assigned a dynamic value: %s', (_, input) => { - const exports = statically_analyse_exports('', input); + const exports = statically_analyse_page_options('', input); expect(exports).toEqual(null); }); @@ -62,7 +62,7 @@ test.each([ ['export all declaration alias', 'export * as bar from "./foo"'], ['non-page option export', "export const foo = 'bar'"] ])('ignores %s', (_, input) => { - const exports = statically_analyse_exports('', input); + const exports = statically_analyse_page_options('', input); expect(exports).toEqual({}); }); @@ -72,7 +72,7 @@ test.each([ ['whitespace', 'export * from "./foo";'], ['multiple lines and whitespace', "export \n *\n from 'abc'; "] ])('fails when export all declaration is used: %s', (_, input) => { - const exports = statically_analyse_exports('', input); + const exports = statically_analyse_page_options('', input); expect(exports).toEqual(null); }); @@ -131,7 +131,7 @@ test.each([ ` ] ])('non-reassigned page options: %s', (_, input) => { - const exports = statically_analyse_exports('', input); + const exports = statically_analyse_page_options('', input); expect(exports).toEqual({ ssr: true, prerender: true }); }); @@ -151,7 +151,7 @@ test.each([ ` ] ])('export specifier references: %s', (_, input) => { - const exports = statically_analyse_exports('', input); + const exports = statically_analyse_page_options('', input); expect(exports).toEqual({ ssr: false }); }); @@ -185,6 +185,6 @@ test.each([ ` ] ])('fails when export specifier references: %s', (_, input) => { - const exports = statically_analyse_exports('', input); + const exports = statically_analyse_page_options('', input); expect(exports).toEqual(null); }); diff --git a/packages/kit/test/apps/writes/src/routes/universal/parent-changed/+layout.js b/packages/kit/test/apps/writes/src/routes/universal/parent-changed/+layout.js new file mode 100644 index 000000000000..a3d15781a772 --- /dev/null +++ b/packages/kit/test/apps/writes/src/routes/universal/parent-changed/+layout.js @@ -0,0 +1 @@ +export const ssr = false; diff --git a/packages/kit/test/apps/writes/src/routes/universal/parent-changed/+page.js b/packages/kit/test/apps/writes/src/routes/universal/parent-changed/+page.js new file mode 100644 index 000000000000..b01f93ba591a --- /dev/null +++ b/packages/kit/test/apps/writes/src/routes/universal/parent-changed/+page.js @@ -0,0 +1 @@ +document.body.style.backgroundColor = 'red'; diff --git a/packages/kit/test/apps/writes/src/routes/universal/parent-changed/+page.svelte b/packages/kit/test/apps/writes/src/routes/universal/parent-changed/+page.svelte new file mode 100644 index 000000000000..48aa4cb69f99 --- /dev/null +++ b/packages/kit/test/apps/writes/src/routes/universal/parent-changed/+page.svelte @@ -0,0 +1 @@ +

hello world

diff --git a/packages/kit/test/apps/writes/test/test.js b/packages/kit/test/apps/writes/test/test.js index 5ebc88f92e75..e513c647f37d 100644 --- a/packages/kit/test/apps/writes/test/test.js +++ b/packages/kit/test/apps/writes/test/test.js @@ -64,8 +64,18 @@ test.describe('Filesystem updates', () => { const file = fileURLToPath(new URL('../src/routes/universal/+page.js', import.meta.url)); const contents = fs.readFileSync(file, 'utf-8'); - await page.goto('/universal'); + try { + fs.writeFileSync(file, contents.replace(/export const ssr = false;\n/, '')); + await page.goto('/universal', { wait_for_started: false }); + expect(await get_computed_style('body', 'background-color')).not.toBe('rgb(255, 0, 0)'); + await expect(page.locator('h1')).toHaveText('Internal Error'); + } finally { + fs.writeFileSync(file, contents); + } + await page.waitForTimeout(500); // this is the rare time we actually need waitForTimeout; we have no visibility into whether the module graph has been invalidated + // a reload is required because Vite HMR doesn't trigger if the page has never loaded successfully + await page.reload(); expect(await get_computed_style('body', 'background-color')).toBe('rgb(255, 0, 0)'); try { @@ -74,10 +84,32 @@ test.describe('Filesystem updates', () => { expect(await get_computed_style('body', 'background-color')).not.toBe('rgb(255, 0, 0)'); await expect(page.locator('h1')).toHaveText('Internal Error'); } finally { - fs.writeFileSync( - file, - contents.replace(/export const ssr = .*;/, 'export const ssr = false;') - ); + fs.writeFileSync(file, contents.replace(/\\nexport const ssr = false;\\n/, '')); + } + }); + + test('Universal node is updated when parent page options change', async ({ + page, + javaScriptEnabled, + get_computed_style + }) => { + test.skip(!process.env.DEV || !javaScriptEnabled); + + const file = fileURLToPath( + new URL('../src/routes/universal/parent-changed/+layout.js', import.meta.url) + ); + const contents = fs.readFileSync(file, 'utf-8'); + + try { + await page.goto('/universal/parent-changed'); + expect(await get_computed_style('body', 'background-color')).toBe('rgb(255, 0, 0)'); + + fs.writeFileSync(file, contents.replace(/export const ssr = false;/, '')); + await page.waitForTimeout(500); // this is the rare time we actually need waitForTimeout; we have no visibility into whether the module graph has been invalidated + expect(await get_computed_style('body', 'background-color')).not.toBe('rgb(255, 0, 0)'); + await expect(page.locator('h1')).toHaveText('Internal Error'); + } finally { + fs.writeFileSync(file, contents); } }); });