Surface configuration errors to the client (#5273)

* Surface configuration errors to the client

* Actually start the container on restart

* Add beforeRestart to clear the console

* Some minor changes, restarted() returns an Error maybe

* Refactor testing code

* Adding a changeset

.changeset/brown-clocks-press.md

@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Surface astro.config errors to the user

packages/astro/src/cli/index.ts

@@ -177,67 +177,21 @@ async function runCommand(cmd: string, flags: yargs.Arguments) {
 	// by the end of this switch statement.
 	switch (cmd) {
 		case 'dev': {
-			async function startDevServer({ isRestart = false }: { isRestart?: boolean } = {}) {
+			const configFlag = resolveFlags(flags).config;
-				const { watcher, stop } = await devServer(settings, { logging, telemetry, isRestart });
+			const configFlagPath = configFlag
-				let restartInFlight = false;
+				? await resolveConfigPath({ cwd: root, flags })
-				const configFlag = resolveFlags(flags).config;
+				: undefined;
-				const configFlagPath = configFlag
-					? await resolveConfigPath({ cwd: root, flags })
-					: undefined;
-				const resolvedRoot = appendForwardSlash(resolveRoot(root));
 
-				const handleServerRestart = (logMsg: string) =>
+			await devServer(settings, {
-					async function (changedFile: string) {
+				configFlag,
-						if (restartInFlight) return;
+				configFlagPath,
-
+				logging,
-						let shouldRestart = false;
+				telemetry,
-
+				handleConfigError(e) {
-						// If the config file changed, reload the config and restart the server.
+					handleConfigError(e, { cwd: root, flags, logging });
-						shouldRestart = configFlag
+					info(logging, 'astro', 'Continuing with previous valid configuration\n');
-							? // If --config is specified, only watch changes for this file
+				}
-							  !!configFlagPath && normalizePath(configFlagPath) === normalizePath(changedFile)
+			});
-							: // Otherwise, watch for any astro.config.* file changes in project root
-							  new RegExp(
-									`${normalizePath(resolvedRoot)}.*astro\.config\.((mjs)|(cjs)|(js)|(ts))$`
-							  ).test(normalizePath(changedFile));
-
-						if (!shouldRestart && settings.watchFiles.length > 0) {
-							// If the config file didn't change, check if any of the watched files changed.
-							shouldRestart = settings.watchFiles.some(
-								(path) => normalizePath(path) === normalizePath(changedFile)
-							);
-						}
-
-						if (!shouldRestart) return;
-
-						restartInFlight = true;
-						console.clear();
-						try {
-							const newConfig = await openConfig({
-								cwd: root,
-								flags,
-								cmd,
-								logging,
-								isRestart: true,
-							});
-							info(logging, 'astro', logMsg + '\n');
-							let astroConfig = newConfig.astroConfig;
-							settings = createSettings(astroConfig, root);
-							await stop();
-							await startDevServer({ isRestart: true });
-						} catch (e) {
-							await handleConfigError(e, { cwd: root, flags, logging });
-							await stop();
-							info(logging, 'astro', 'Continuing with previous valid configuration\n');
-							await startDevServer({ isRestart: true });
-						}
-					};
-
-				watcher.on('change', handleServerRestart('Configuration updated. Restarting...'));
-				watcher.on('unlink', handleServerRestart('Configuration removed. Restarting...'));
-				watcher.on('add', handleServerRestart('Configuration added. Restarting...'));
-			}
-			await startDevServer({ isRestart: false });
 			return await new Promise(() => {}); // lives forever
 		}
 

packages/astro/src/core/config/config.ts

@@ -105,7 +105,10 @@ export function resolveFlags(flags: Partial<Flags>): CLIFlags {
 	};
 }
 
-export function resolveRoot(cwd?: string): string {
+export function resolveRoot(cwd?: string | URL): string {
+	if(cwd instanceof URL) {
+		cwd = fileURLToPath(cwd);
+	}
 	return cwd ? path.resolve(cwd) : process.cwd();
 }
 
@@ -137,6 +140,7 @@ interface LoadConfigOptions {
 	logging: LogOptions;
 	/** Invalidate when reloading a previously loaded config */
 	isRestart?: boolean;
+	fsMod?: typeof fs;
 }
 
 /**
@@ -210,6 +214,7 @@ async function tryLoadConfig(
 	flags: CLIFlags,
 	root: string
 ): Promise<TryLoadConfigResult | undefined> {
+	const fsMod = configOptions.fsMod ?? fs;
 	let finallyCleanup = async () => {};
 	try {
 		let configPath = await resolveConfigPath({
@@ -224,7 +229,9 @@ async function tryLoadConfig(
 				root,
 				`.temp.${Date.now()}.config${path.extname(configPath)}`
 			);
-			await fs.promises.writeFile(tempConfigPath, await fs.promises.readFile(configPath));
+
+			const currentConfigContent = await fsMod.promises.readFile(configPath, 'utf-8');
+			await fs.promises.writeFile(tempConfigPath, currentConfigContent);
 			finallyCleanup = async () => {
 				try {
 					await fs.promises.unlink(tempConfigPath);

packages/astro/src/core/dev/container.ts

@@ -9,10 +9,12 @@ import {
 	runHookConfigSetup,
 	runHookServerSetup,
 	runHookServerStart,
+	runHookServerDone
 } from '../../integrations/index.js';
-import { createDefaultDevSettings } from '../config/index.js';
+import { createDefaultDevSettings, resolveRoot } from '../config/index.js';
 import { createVite } from '../create-vite.js';
 import { LogOptions } from '../logger/core.js';
+import { appendForwardSlash } from '../path.js';
 import { nodeLogDestination } from '../logger/node.js';
 import { apply as applyPolyfill } from '../polyfill.js';
 
@@ -27,6 +29,10 @@ export interface Container {
 	settings: AstroSettings;
 	viteConfig: vite.InlineConfig;
 	viteServer: vite.ViteDevServer;
+	resolvedRoot: string;
+	configFlag: string | undefined;
+	configFlagPath: string | undefined;
+	restartInFlight: boolean; // gross
 	handle: (req: http.IncomingMessage, res: http.ServerResponse) => void;
 	close: () => Promise<void>;
 }
@@ -38,6 +44,9 @@ export interface CreateContainerParams {
 	settings?: AstroSettings;
 	fs?: typeof nodeFs;
 	root?: string | URL;
+	// The string passed to --config and the resolved path
+	configFlag?: string;
+	configFlagPath?: string;
 }
 
 export async function createContainer(params: CreateContainerParams = {}): Promise<Container> {
@@ -83,20 +92,38 @@ export async function createContainer(params: CreateContainerParams = {}): Promi
 	const viteServer = await vite.createServer(viteConfig);
 	runHookServerSetup({ config: settings.config, server: viteServer, logging });
 
-	return {
+	const container: Container = {
+		configFlag: params.configFlag,
+		configFlagPath: params.configFlagPath,
 		fs,
 		logging,
+		resolvedRoot: appendForwardSlash(resolveRoot(params.root)),
+		restartInFlight: false,
 		settings,
 		viteConfig,
 		viteServer,
-
 		handle(req, res) {
 			viteServer.middlewares.handle(req, res, Function.prototype);
 		},
+		// TODO deprecate and remove
 		close() {
-			return viteServer.close();
+			return closeContainer(container);
-		},
+		}
 	};
+
+	return container;
+}
+
+async function closeContainer({
+	viteServer,
+	settings,
+	logging
+}: Container) {
+	await viteServer.close();
+	await runHookServerDone({
+		config: settings.config,
+		logging,
+	});
 }
 
 export async function startContainer({
@@ -116,6 +143,10 @@ export async function startContainer({
 	return devServerAddressInfo;
 }
 
+export function isStarted(container: Container): boolean {
+	return !!container.viteServer.httpServer?.listening;
+}
+
 export async function runInContainer(
 	params: CreateContainerParams,
 	callback: (container: Container) => Promise<void> | void

packages/astro/src/core/dev/dev.ts

@@ -1,21 +1,26 @@
 import type { AstroTelemetry } from '@astrojs/telemetry';
 import type { AddressInfo } from 'net';
+import type http from 'http';
 import { performance } from 'perf_hooks';
 import * as vite from 'vite';
 import type { AstroSettings } from '../../@types/astro';
-import { runHookServerDone } from '../../integrations/index.js';
 import { info, LogOptions, warn } from '../logger/core.js';
 import * as msg from '../messages.js';
-import { createContainer, startContainer } from './container.js';
+import { startContainer } from './container.js';
+import { createContainerWithAutomaticRestart } from './restart.js';
 
 export interface DevOptions {
+	configFlag: string | undefined;
+	configFlagPath: string | undefined;
 	logging: LogOptions;
 	telemetry: AstroTelemetry;
+	handleConfigError: (error: Error) => void;
 	isRestart?: boolean;
 }
 
 export interface DevServer {
 	address: AddressInfo;
+	handle: (req: http.IncomingMessage, res: http.ServerResponse<http.IncomingMessage>) => void;
 	watcher: vite.FSWatcher;
 	stop(): Promise<void>;
 }
@@ -29,14 +34,20 @@ export default async function dev(
 	await options.telemetry.record([]);
 
 	// Create a container which sets up the Vite server.
-	const container = await createContainer({
+	const restart = await createContainerWithAutomaticRestart({
-		settings,
+		flags: {},
-		logging: options.logging,
+		handleConfigError: options.handleConfigError,
-		isRestart: options.isRestart,
+		// eslint-disable-next-line no-console
+		beforeRestart: () => console.clear(),
+		params: {
+			settings,
+			logging: options.logging,
+			isRestart: options.isRestart,
+		}
 	});
 
 	// Start listening to the port
-	const devServerAddressInfo = await startContainer(container);
+	const devServerAddressInfo = await startContainer(restart.container);
 
 	const site = settings.config.site
 		? new URL(settings.config.base, settings.config.site)
@@ -46,7 +57,7 @@ export default async function dev(
 		null,
 		msg.serverStart({
 			startupTime: performance.now() - devStart,
-			resolvedUrls: container.viteServer.resolvedUrls || { local: [], network: [] },
+			resolvedUrls: restart.container.viteServer.resolvedUrls || { local: [], network: [] },
 			host: settings.config.server.host,
 			site,
 			isRestart: options.isRestart,
@@ -57,18 +68,20 @@ export default async function dev(
 	if (currentVersion.includes('-')) {
 		warn(options.logging, null, msg.prerelease({ currentVersion }));
 	}
-	if (container.viteConfig.server?.fs?.strict === false) {
+	if (restart.container.viteConfig.server?.fs?.strict === false) {
 		warn(options.logging, null, msg.fsStrictWarning());
 	}
 
 	return {
 		address: devServerAddressInfo,
 		get watcher() {
-			return container.viteServer.watcher;
+			return restart.container.viteServer.watcher;
 		},
-		stop: async () => {
+		handle(req, res) {
-			await container.close();
+			return restart.container.handle(req, res);
-			await runHookServerDone({ config: settings.config, logging: options.logging });
+		},
+		async stop() {
+			await restart.container.close();
 		},
 	};
 }

packages/astro/src/core/dev/index.ts

@@ -1,2 +1,9 @@
-export { createContainer, runInContainer, startContainer } from './container.js';
+export {
+	createContainer,
+	runInContainer,
+	startContainer
+} from './container.js';
+export {
+	createContainerWithAutomaticRestart
+} from './restart.js';
 export { default } from './dev.js';

packages/astro/src/core/dev/restart.ts

@@ -0,0 +1,197 @@
+import type { AstroSettings } from '../../@types/astro';
+import type { Container, CreateContainerParams } from './container';
+import * as vite from 'vite';
+import { createSettings, openConfig } from '../config/index.js';
+import { info } from '../logger/core.js';
+import { createContainer, isStarted, startContainer } from './container.js';
+import { createSafeError } from '../errors/index.js';
+
+async function createRestartedContainer(container: Container, settings: AstroSettings): Promise<Container> {
+	const {
+		logging,
+		fs,
+		resolvedRoot,
+		configFlag,
+		configFlagPath
+	} = container;
+	const needsStart = isStarted(container);
+	const newContainer = await createContainer({
+		isRestart: true,
+		logging,
+		settings,
+		fs,
+		root: resolvedRoot,
+		configFlag,
+		configFlagPath,
+	});
+
+	if(needsStart) {
+		await startContainer(newContainer);
+	}
+
+	return newContainer;
+}
+
+export function shouldRestartContainer({
+	settings,
+	configFlag,
+	configFlagPath,
+	restartInFlight
+}: Container, changedFile: string): boolean {
+	if(restartInFlight) return false;
+
+	let shouldRestart = false;
+
+	// If the config file changed, reload the config and restart the server.
+	if(configFlag) {
+		if(!!configFlagPath) {
+			shouldRestart = vite.normalizePath(configFlagPath) === vite.normalizePath(changedFile);
+		}
+	}
+	// Otherwise, watch for any astro.config.* file changes in project root
+	else {
+		const exp = new RegExp(`.*astro\.config\.((mjs)|(cjs)|(js)|(ts))$`);
+		const normalizedChangedFile = vite.normalizePath(changedFile);
+		shouldRestart = exp.test(normalizedChangedFile);
+	}
+
+	if (!shouldRestart && settings.watchFiles.length > 0) {
+		// If the config file didn't change, check if any of the watched files changed.
+		shouldRestart = settings.watchFiles.some(
+			(path) => vite.normalizePath(path) === vite.normalizePath(changedFile)
+		);
+	}
+
+	return shouldRestart;
+}
+
+interface RestartContainerParams {
+	container: Container;
+	flags: any;
+	logMsg: string;
+	handleConfigError: (err: Error) => Promise<void> | void;
+	beforeRestart?: () => void;
+}
+
+export async function restartContainer({
+	container,
+	flags,
+	logMsg,
+	handleConfigError,
+	beforeRestart
+}: RestartContainerParams): Promise<{ container: Container, error: Error | null }> {
+	const {
+		logging,
+		close,
+		resolvedRoot,
+		settings: existingSettings
+	} = container;
+	container.restartInFlight = true;
+
+	//console.clear(); // TODO move this
+	if(beforeRestart) {
+		beforeRestart()
+	}
+	try {
+		const newConfig = await openConfig({
+			cwd: resolvedRoot,
+			flags,
+			cmd: 'dev',
+			logging,
+			isRestart: true,
+			fsMod: container.fs,
+		});
+		info(logging, 'astro', logMsg + '\n');
+		let astroConfig = newConfig.astroConfig;
+		const settings = createSettings(astroConfig, resolvedRoot);
+		await close();
+		return {
+			container: await createRestartedContainer(container, settings),
+			error: null
+		};
+	} catch (_err) {
+		const error = createSafeError(_err);
+		await handleConfigError(error);
+		await close();
+		info(logging, 'astro', 'Continuing with previous valid configuration\n');
+		return {
+			container: await createRestartedContainer(container, existingSettings),
+			error
+		};
+	}
+}
+
+export interface CreateContainerWithAutomaticRestart {
+	flags: any;
+	params: CreateContainerParams;
+	handleConfigError?: (error: Error) => void | Promise<void>;
+	beforeRestart?: () => void;
+}
+
+interface Restart {
+	container: Container;
+	restarted: () => Promise<Error | null>;
+}
+
+export async function createContainerWithAutomaticRestart({
+	flags,
+	handleConfigError = (_e: Error) => {},
+	beforeRestart,
+	params
+}: CreateContainerWithAutomaticRestart): Promise<Restart> {
+	const initialContainer = await createContainer(params);
+	let resolveRestart: (value: Error | null) => void;
+	let restartComplete = new Promise<Error | null>(resolve => {
+		resolveRestart = resolve;
+	});
+
+	let restart: Restart = {
+		container: initialContainer,
+		restarted() {
+			return restartComplete;
+		}
+	};
+
+	function handleServerRestart(logMsg: string) {
+		// eslint-disable-next-line @typescript-eslint/no-shadow
+		const container = restart.container;
+		return async function(changedFile: string) {
+			if(shouldRestartContainer(container, changedFile)) {
+				const { container: newContainer, error } = await restartContainer({
+					beforeRestart,
+					container,
+					flags,
+					logMsg,
+					async handleConfigError(err) {
+						// Send an error message to the client if one is connected.
+						await handleConfigError(err);
+						container.viteServer.ws.send({
+							type: 'error',
+							err: {
+								message: err.message,
+								stack: err.stack || ''
+							}
+						});
+					}
+				});
+				restart.container = newContainer;
+				// Add new watches because this is a new container with a new Vite server
+				addWatches();
+				resolveRestart(error);
+				restartComplete = new Promise<Error | null>(resolve => {
+					resolveRestart = resolve;
+				});
+			}
+		}
+	}
+
+	// Set up watches
+	function addWatches() {
+		const watcher = restart.container.viteServer.watcher;
+		watcher.on('change', handleServerRestart('Configuration updated. Restarting...'));
+		watcher.on('unlink', handleServerRestart('Configuration removed. Restarting...'));
+		watcher.on('add', handleServerRestart('Configuration added. Restarting...'));
+	}
+	addWatches();
+	return restart;
+}

packages/astro/test/units/dev/restart.test.js

@@ -0,0 +1,68 @@
+import { expect } from 'chai';
+import * as cheerio from 'cheerio';
+
+import { createContainerWithAutomaticRestart, runInContainer } from '../../../dist/core/dev/index.js';
+import { createFs, createRequestAndResponse } from '../test-utils.js';
+
+const root = new URL('../../fixtures/alias/', import.meta.url);
+
+describe('dev container restarts', () => {
+	it('Surfaces config errors on restarts', async () => {
+		const fs = createFs(
+			{
+				'/src/pages/index.astro': `
+				<html>
+					<head><title>Test</title></head>
+					<body>
+						<h1>Test</h1>
+					</body>
+				</html>
+			`,
+				'/astro.config.mjs': `
+				
+				`
+			},
+			root
+		);
+
+		let restart = await createContainerWithAutomaticRestart({
+			params: { fs, root }
+		});
+
+		try {
+			let r = createRequestAndResponse({
+				method: 'GET',
+				url: '/',
+			});
+			restart.container.handle(r.req, r.res);
+			let html = await r.text();
+			const $ = cheerio.load(html);
+			expect(r.res.statusCode).to.equal(200);
+			expect($('h1')).to.have.a.lengthOf(1);
+	
+			// Create an error
+			let restartComplete = restart.restarted();
+			fs.writeFileFromRootSync('/astro.config.mjs', 'const foo = bar');
+
+			// Vite watches the real filesystem, so we have to mock this part. It's not so bad.
+			restart.container.viteServer.watcher.emit('change', fs.getFullyResolvedPath('/astro.config.mjs'));
+
+			// Wait for the restart to finish
+			let hmrError = await restartComplete;
+			expect(hmrError).to.not.be.a('undefined');
+
+			// Do it a second time to make sure we are still watching
+
+			restartComplete = restart.restarted();
+			fs.writeFileFromRootSync('/astro.config.mjs', 'const foo = bar2');
+
+			// Vite watches the real filesystem, so we have to mock this part. It's not so bad.
+			restart.container.viteServer.watcher.emit('change', fs.getFullyResolvedPath('/astro.config.mjs'));
+
+			hmrError = await restartComplete;
+			expect(hmrError).to.not.be.a('undefined');
+		} finally {
+			await restart.container.close();
+		}
+	});
+});

packages/astro/test/units/test-utils.js

@@ -6,12 +6,26 @@ import npath from 'path';
 import { unixify } from './correct-path.js';
 
 class MyVolume extends Volume {
+	#root = '';
+	constructor(root) {
+		super();
+		this.#root = root;
+	}
+
+	getFullyResolvedPath(pth) {
+		return npath.posix.join(this.#root, pth);
+	}
+
 	existsSync(p) {
 		if (p instanceof URL) {
 			p = fileURLToPath(p);
 		}
 		return super.existsSync(p);
 	}
+
+	writeFileFromRootSync(pth, ...rest) {
+		return super.writeFileSync(this.getFullyResolvedPath(pth), ...rest);
+	}
 }
 
 export function createFs(json, root) {
@@ -25,7 +39,7 @@ export function createFs(json, root) {
 		structure[fullpath] = value;
 	}
 
-	const fs = new MyVolume();
+	const fs = new MyVolume(root);
 	fs.fromJSON(structure);
 	return fs;
 }