Restyle error messages (#2902)

* clean up error logging in astro * update from ben feedback * Update packages/astro/src/core/build/generate.ts Co-authored-by: Ben Holmes <hey@bholmes.dev> Co-authored-by: Ben Holmes <hey@bholmes.dev>
withastro · Mar 28, 2022 · f702070 · f702070
1 parent b7ee855
commit f702070
Show file tree

Hide file tree

Showing 21 changed files with 368 additions and 346 deletions.
diff --git a/.changeset/small-radios-remain.md b/.changeset/small-radios-remain.md
@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Update CLI error format and style
diff --git a/packages/astro/src/cli/check.ts b/packages/astro/src/cli/check.ts
@@ -57,7 +57,7 @@ function offsetAt({ line, character }: { line: number; character: number }, text
 	return i;
 }
 
-function pad(str: string, len: number) {
+function generateString(str: string, len: number) {
 	return Array.from({ length: len }, () => str).join('');
 }
 
@@ -86,8 +86,8 @@ export async function check(astroConfig: AstroConfig) {
 					const lineNumStr = d.range.start.line.toString();
 					const lineNumLen = lineNumStr.length;
 					console.error(`${bgWhite(black(lineNumStr))}  ${str}`);
-					let tildes = pad('~', d.range.end.character - d.range.start.character);
-					let spaces = pad(' ', d.range.start.character + lineNumLen - 1);
+					let tildes = generateString('~', d.range.end.character - d.range.start.character);
+					let spaces = generateString(' ', d.range.start.character + lineNumLen - 1);
 					console.error(`   ${spaces}${bold(red(tildes))}\n`);
 					result.errors++;
 					break;

diff --git a/packages/astro/src/cli/index.ts b/packages/astro/src/cli/index.ts
@@ -12,8 +12,9 @@ import add from '../core/add/index.js';
 import devServer from '../core/dev/index.js';
 import preview from '../core/preview/index.js';
 import { check } from './check.js';
-import { formatConfigError, loadConfig } from '../core/config.js';
-import { printHelp } from '../core/messages.js';
+import { loadConfig } from '../core/config.js';
+import { printHelp, formatErrorMessage, formatConfigErrorMessage } from '../core/messages.js';
+import { createSafeError } from '../core/util.js';
 
 type Arguments = yargs.Arguments;
 type CLICommand = 'help' | 'version' | 'add' | 'dev' | 'build' | 'preview' | 'reload' | 'check';
@@ -102,40 +103,33 @@ export async function cli(args: string[]) {
 		// For now, `add` has to resolve the config again internally
 		config = await loadConfig({ cwd: projectRoot, flags });
 	} catch (err) {
-		throwAndExit(err);
-		return;
+		return throwAndExit(err);
 	}
 
 	switch (cmd) {
 		case 'add': {
 			try {
 				const packages = flags._.slice(3) as string[];
-				await add(packages, { cwd: projectRoot, flags, logging });
-				process.exit(0);
+				return await add(packages, { cwd: projectRoot, flags, logging });
 			} catch (err) {
-				throwAndExit(err);
+				return throwAndExit(err);
 			}
-			return;
 		}
 		case 'dev': {
 			try {
 				await devServer(config, { logging });
-
-				await new Promise(() => {}); // don’t close dev server
+				return await new Promise(() => {}); // lives forever
 			} catch (err) {
-				throwAndExit(err);
+				return throwAndExit(err);
 			}
-			return;
 		}
 
 		case 'build': {
 			try {
-				await build(config, { logging });
-				process.exit(0);
+				return await build(config, { logging });
 			} catch (err) {
-				throwAndExit(err);
+				return throwAndExit(err);
 			}
-			return;
 		}
 
 		case 'check': {
@@ -145,11 +139,10 @@ export async function cli(args: string[]) {
 
 		case 'preview': {
 			try {
-				await preview(config, { logging }); // this will keep running
+				return await preview(config, { logging }); // this will keep running
 			} catch (err) {
-				throwAndExit(err);
+				return throwAndExit(err);
 			}
-			return;
 		}
 
 		default: {
@@ -159,14 +152,11 @@ export async function cli(args: string[]) {
 }
 
 /** Display error and exit */
-function throwAndExit(err: any) {
+function throwAndExit(err: unknown) {
 	if (err instanceof z.ZodError) {
-		console.error(formatConfigError(err));
-	} else if (err.stack) {
-		const [mainMsg, ...stackMsg] = err.stack.split('\n');
-		console.error(colors.red(mainMsg) + '\n' + colors.dim(stackMsg.join('\n')));
+		console.error(formatConfigErrorMessage(err));
 	} else {
-		console.error(colors.red(err.toString() || err));
+		console.error(formatErrorMessage(createSafeError(err)));
 	}
 	process.exit(1);
 }
diff --git a/packages/astro/src/core/add/index.ts b/packages/astro/src/core/add/index.ts
@@ -54,6 +54,7 @@ export default async function add(names: string[], { cwd, flags, logging }: AddO
 	}
 	applyPolyfill();
 
+	// If no integrations were given, prompt the user for some popular ones.
 	if (names.length === 0) {
 		const response = await prompts([
 			{
@@ -72,16 +73,13 @@ export default async function add(names: string[], { cwd, flags, logging }: AddO
 			},
 		]);
 
-		if (!response.frameworks && !response.addons) {
-			info(logging, null, msg.cancelled(`Integrations skipped.`, `You can always run ${cyan('astro add')} later!`));
-			return;
-		}
-		const selected = [response.frameworks ?? [], response.addons ?? []].flat(1);
-		if (selected.length === 0) {
-			error(logging, null, `\n${red('No integrations specified!')}\n${dim('Try running')} astro add again.`);
-			return;
-		}
-		names = selected;
+		names = [...(response.frameworks ?? []), ...(response.addons ?? [])];
+	}
+
+	// If still empty after prompting, exit gracefully.
+	if (names.length === 0) {
+		error(logging, null, `No integrations specified.`);
+		return;
 	}
 
 	// Some packages might have a common alias! We normalize those here.

diff --git a/packages/astro/src/core/build/common.ts b/packages/astro/src/core/build/common.ts
@@ -4,7 +4,7 @@ import { appendForwardSlash } from '../../core/path.js';
 
 const STATUS_CODE_PAGES = new Set(['/404', '/500']);
 
-export function getOutRoot(astroConfig: AstroConfig): URL {
+function getOutRoot(astroConfig: AstroConfig): URL {
 	return new URL('./', astroConfig.dist);
 }
 

diff --git a/packages/astro/src/core/build/generate.ts b/packages/astro/src/core/build/generate.ts
@@ -1,21 +1,21 @@
+import fs from 'fs';
+import { bgGreen, bgMagenta, black, cyan, dim, green, magenta } from 'kleur/colors';
+import npath from 'path';
 import type { OutputAsset, OutputChunk, RollupOutput } from 'rollup';
+import { fileURLToPath } from 'url';
 import type { AstroConfig, ComponentInstance, EndpointHandler, SSRLoadedRenderer } from '../../@types/astro';
-import type { PageBuildData, StaticBuildOptions, SingleFileBuiltModule } from './types';
 import type { BuildInternals } from '../../core/build/internal.js';
+import { debug, info } from '../../core/logger.js';
+import { appendForwardSlash, prependForwardSlash } from '../../core/path.js';
 import type { RenderOptions } from '../../core/render/core';
-
-import fs from 'fs';
-import npath from 'path';
-import { fileURLToPath } from 'url';
-import { debug, error, info } from '../../core/logger.js';
-import { prependForwardSlash } from '../../core/path.js';
 import { BEFORE_HYDRATION_SCRIPT_ID } from '../../vite-plugin-scripts/index.js';
 import { call as callEndpoint } from '../endpoint/index.js';
 import { render } from '../render/core.js';
 import { createLinkStylesheetElementSet, createModuleScriptElementWithSrcSet } from '../render/ssr-element.js';
-import { getOutFile, getOutRoot, getOutFolder } from './common.js';
-import { getPageDataByComponent, eachPageData } from './internal.js';
-import { bgMagenta, black, cyan, dim, magenta } from 'kleur/colors';
+import { getOutputFilename } from '../util.js';
+import { getOutFile, getOutFolder } from './common.js';
+import { eachPageData, getPageDataByComponent } from './internal.js';
+import type { PageBuildData, SingleFileBuiltModule, StaticBuildOptions } from './types';
 import { getTimeStat } from './util.js';
 
 // Render is usually compute, which Node.js can't parallelize well.
@@ -67,7 +67,8 @@ export function chunkIsPage(astroConfig: AstroConfig, output: OutputAsset | Outp
 }
 
 export async function generatePages(result: RollupOutput, opts: StaticBuildOptions, internals: BuildInternals, facadeIdToPageDataMap: Map<string, PageBuildData>) {
-	info(opts.logging, null, `\n${bgMagenta(black(' generating static routes '))}\n`);
+	const timer = performance.now();
+	info(opts.logging, null, `\n${bgGreen(black(' generating static routes '))}`);
 
 	const ssr = !!opts.astroConfig._ctx.adapter?.serverEntrypoint;
 	const serverEntry = opts.buildConfig.serverEntry;
@@ -78,6 +79,7 @@ export async function generatePages(result: RollupOutput, opts: StaticBuildOptio
 	for (const pageData of eachPageData(internals)) {
 		await generatePage(opts, internals, pageData, ssrEntry);
 	}
+	info(opts.logging, null, dim(`Completed in ${getTimeStat(timer, performance.now())}.\n`));
 }
 
 async function generatePage(
@@ -109,31 +111,18 @@ async function generatePage(
 		renderers,
 	};
 
-	const icon = pageData.route.type === 'page' ? cyan('</>') : magenta('{-}');
+	const icon = pageData.route.type === 'page' ? green('▶') : magenta('λ');
 	info(opts.logging, null, `${icon} ${pageData.route.component}`);
 
-	// Throttle the paths to avoid overloading the CPU with too many tasks.
-	const renderPromises = [];
-	for (const paths of throttle(MAX_CONCURRENT_RENDERS, pageData.paths)) {
-		for (const path of paths) {
-			renderPromises.push(generatePath(path, opts, generationOptions));
-		}
-		// This blocks generating more paths until these 10 complete.
-		await Promise.all(renderPromises);
+	for (let i = 0; i < pageData.paths.length; i++) {
+		const path = pageData.paths[i];
+		await generatePath(path, opts, generationOptions);
 		const timeEnd = performance.now();
 		const timeChange = getTimeStat(timeStart, timeEnd);
-		let shouldLogTimeChange = !getTimeStat(timeStart, timeEnd).startsWith('0');
-		for (const path of paths) {
-			const timeIncrease = shouldLogTimeChange ? ` ${dim(`+${timeChange}`)}` : '';
-			info(opts.logging, null, `    ${dim('┃')} ${path}${timeIncrease}`);
-			// Should only log build time on the first generated path
-			// Logging for all generated paths adds extra noise
-			shouldLogTimeChange = false;
-		}
-		// Reset timeStart for the next batch of rendered paths
-		timeStart = performance.now();
-		// This empties the array without allocating a new one.
-		renderPromises.length = 0;
+		const timeIncrease = `(+${timeChange})`;
+		const filePath = getOutputFilename(opts.astroConfig, path);
+		const lineIcon = i === pageData.paths.length - 1 ? '└─' : '├─';
+		info(opts.logging, null, `  ${cyan(lineIcon)} ${dim(filePath)} ${dim(timeIncrease)}`);
 	}
 }
 
@@ -175,65 +164,61 @@ async function generatePath(pathname: string, opts: StaticBuildOptions, gopts: G
 		}
 	}
 
-	try {
-		const options: RenderOptions = {
-			legacyBuild: false,
-			links,
-			logging,
-			markdownRender: astroConfig.markdownOptions.render,
-			mod,
-			origin,
-			pathname,
-			scripts,
-			renderers,
-			async resolve(specifier: string) {
-				const hashedFilePath = internals.entrySpecifierToBundleMap.get(specifier);
-				if (typeof hashedFilePath !== 'string') {
-					// If no "astro:scripts/before-hydration.js" script exists in the build,
-					// then we can assume that no before-hydration scripts are needed.
-					// Return this as placeholder, which will be ignored by the browser.
-					// TODO: In the future, we hope to run this entire script through Vite,
-					// removing the need to maintain our own custom Vite-mimic resolve logic.
-					if (specifier === BEFORE_HYDRATION_SCRIPT_ID) {
-						return 'data:text/javascript;charset=utf-8,//[no before-hydration script]';
-					}
-					throw new Error(`Cannot find the built path for ${specifier}`);
+	const options: RenderOptions = {
+		legacyBuild: false,
+		links,
+		logging,
+		markdownRender: astroConfig.markdownOptions.render,
+		mod,
+		origin,
+		pathname,
+		scripts,
+		renderers,
+		async resolve(specifier: string) {
+			const hashedFilePath = internals.entrySpecifierToBundleMap.get(specifier);
+			if (typeof hashedFilePath !== 'string') {
+				// If no "astro:scripts/before-hydration.js" script exists in the build,
+				// then we can assume that no before-hydration scripts are needed.
+				// Return this as placeholder, which will be ignored by the browser.
+				// TODO: In the future, we hope to run this entire script through Vite,
+				// removing the need to maintain our own custom Vite-mimic resolve logic.
+				if (specifier === BEFORE_HYDRATION_SCRIPT_ID) {
+					return 'data:text/javascript;charset=utf-8,//[no before-hydration script]';
 				}
-				const relPath = npath.posix.relative(pathname, '/' + hashedFilePath);
-				const fullyRelativePath = relPath[0] === '.' ? relPath : './' + relPath;
-				return fullyRelativePath;
-			},
-			method: 'GET',
-			headers: new Headers(),
-			route: pageData.route,
-			routeCache,
-			site: astroConfig.buildOptions.site,
-			ssr: opts.astroConfig.buildOptions.experimentalSsr,
-		};
-
-		let body: string;
-		if (pageData.route.type === 'endpoint') {
-			const result = await callEndpoint(mod as unknown as EndpointHandler, options);
-
-			if (result.type === 'response') {
-				throw new Error(`Returning a Response from an endpoint is not supported in SSG mode.`);
+				throw new Error(`Cannot find the built path for ${specifier}`);
 			}
-			body = result.body;
-		} else {
-			const result = await render(options);
+			const relPath = npath.posix.relative(pathname, '/' + hashedFilePath);
+			const fullyRelativePath = relPath[0] === '.' ? relPath : './' + relPath;
+			return fullyRelativePath;
+		},
+		method: 'GET',
+		headers: new Headers(),
+		route: pageData.route,
+		routeCache,
+		site: astroConfig.buildOptions.site,
+		ssr: opts.astroConfig.buildOptions.experimentalSsr,
+	};
 
-			// If there's a redirect or something, just do nothing.
-			if (result.type !== 'html') {
-				return;
-			}
-			body = result.html;
+	let body: string;
+	if (pageData.route.type === 'endpoint') {
+		const result = await callEndpoint(mod as unknown as EndpointHandler, options);
+
+		if (result.type === 'response') {
+			throw new Error(`Returning a Response from an endpoint is not supported in SSG mode.`);
 		}
+		body = result.body;
+	} else {
+		const result = await render(options);
 
-		const outFolder = getOutFolder(astroConfig, pathname, pageData.route.type);
-		const outFile = getOutFile(astroConfig, outFolder, pathname, pageData.route.type);
-		await fs.promises.mkdir(outFolder, { recursive: true });
-		await fs.promises.writeFile(outFile, body, 'utf-8');
-	} catch (err) {
-		error(opts.logging, 'build', `Error rendering:`, err);
+		// If there's a redirect or something, just do nothing.
+		if (result.type !== 'html') {
+			return;
+		}
+		body = result.html;
 	}
+
+	const outFolder = getOutFolder(astroConfig, pathname, pageData.route.type);
+	const outFile = getOutFile(astroConfig, outFolder, pathname, pageData.route.type);
+	await fs.promises.mkdir(outFolder, { recursive: true });
+	await fs.promises.writeFile(outFile, body, 'utf-8');
 }