From 31ec8479721a1cd65538ec041458c5ffe8f50ee9 Mon Sep 17 00:00:00 2001 From: Erika <3019731+Princesseuh@users.noreply.github.com> Date: Wed, 14 Dec 2022 11:27:15 -0400 Subject: [PATCH] Add a new error overlay (#5495) * Add new overlay * Fix CSS errors missing the proper stacktrace * Fix names not working in some cases * Add changeset * Fix Playwright not detecting the overlay * Update E2E test * Fix tests * Small refactor, fix syntax highlight on light mode, fix code element showing even with no code * Simplier injection * Add Markdown support to CLI reporting * Fix not being able to navigate with the keyboard to the open in editor link * aria-hide some svgs (#5508) we should also make the "open in editor" button a button, not a link, if we are using JS for interactions * Implement close method so Vite can close the overlay when needed * Fix filepaths not being absolute when coming from node_modules for errors * Fix multi line errors with indentation not showing correctly * Fix entire page being scrolled to the error line in certain cases * Update docs links * Put the new error overlay behind a flag * add flag for e2e tests Co-authored-by: Caleb Jasik Co-authored-by: Matthew Phillips --- .changeset/happy-chefs-ring.md | 5 + packages/astro/e2e/error-cyclic.test.js | 9 +- .../astro/e2e/error-react-spectrum.test.js | 9 +- packages/astro/e2e/error-sass.test.js | 9 +- packages/astro/e2e/errors.test.js | 17 +- packages/astro/e2e/shared-component-tests.js | 2 +- packages/astro/e2e/test-utils.js | 12 +- packages/astro/src/@types/astro.ts | 7 + packages/astro/src/core/compile/style.ts | 3 + packages/astro/src/core/config/config.ts | 2 + packages/astro/src/core/config/schema.ts | 2 + packages/astro/src/core/errors/dev/utils.ts | 79 ++- packages/astro/src/core/errors/dev/vite.ts | 77 ++- packages/astro/src/core/errors/errors-data.ts | 4 +- packages/astro/src/core/errors/errors.ts | 8 +- packages/astro/src/core/errors/overlay.ts | 585 ++++++++++++++++++ packages/astro/src/core/messages.ts | 17 +- .../src/vite-plugin-astro-server/plugin.ts | 22 +- .../src/vite-plugin-astro-server/response.ts | 4 +- 19 files changed, 800 insertions(+), 73 deletions(-) create mode 100644 .changeset/happy-chefs-ring.md create mode 100644 packages/astro/src/core/errors/overlay.ts diff --git a/.changeset/happy-chefs-ring.md b/.changeset/happy-chefs-ring.md new file mode 100644 index 000000000000..c8ac61ddbdd7 --- /dev/null +++ b/.changeset/happy-chefs-ring.md @@ -0,0 +1,5 @@ +--- +'astro': minor +--- + +Add a new error overlay designed by @doodlemarks! This new overlay should be much more informative, clearer, astro-y, and prettier than the previous one. diff --git a/packages/astro/e2e/error-cyclic.test.js b/packages/astro/e2e/error-cyclic.test.js index 78c3bd1ead12..ef17a32d3881 100644 --- a/packages/astro/e2e/error-cyclic.test.js +++ b/packages/astro/e2e/error-cyclic.test.js @@ -1,7 +1,10 @@ import { expect } from '@playwright/test'; -import { testFactory, getErrorOverlayMessage } from './test-utils.js'; +import { testFactory, getErrorOverlayContent } from './test-utils.js'; -const test = testFactory({ root: './fixtures/error-cyclic/' }); +const test = testFactory({ + experimentalErrorOverlay: true, + root: './fixtures/error-cyclic/' +}); let devServer; @@ -18,7 +21,7 @@ test.describe('Error: Cyclic Reference', () => { test('overlay', async ({ page, astro }) => { await page.goto(astro.resolveUrl('/')); - const message = await getErrorOverlayMessage(page); + const message = (await getErrorOverlayContent(page)).message; expect(message).toMatch('Cyclic reference'); }); }); diff --git a/packages/astro/e2e/error-react-spectrum.test.js b/packages/astro/e2e/error-react-spectrum.test.js index 63934c9ca902..618859ac19fb 100644 --- a/packages/astro/e2e/error-react-spectrum.test.js +++ b/packages/astro/e2e/error-react-spectrum.test.js @@ -1,7 +1,10 @@ import { expect } from '@playwright/test'; -import { testFactory, getErrorOverlayMessage } from './test-utils.js'; +import { testFactory, getErrorOverlayContent } from './test-utils.js'; -const test = testFactory({ root: './fixtures/error-react-spectrum/' }); +const test = testFactory({ + experimentalErrorOverlay: true, + root: './fixtures/error-react-spectrum/' +}); let devServer; @@ -17,7 +20,7 @@ test.describe('Error: React Spectrum', () => { test('overlay', async ({ page, astro }) => { await page.goto(astro.resolveUrl('/')); - const message = await getErrorOverlayMessage(page); + const message = (await getErrorOverlayContent(page)).hint; expect(message).toMatch('@adobe/react-spectrum is not compatible'); }); }); diff --git a/packages/astro/e2e/error-sass.test.js b/packages/astro/e2e/error-sass.test.js index e7b87e1052ea..2eeab13dfd0b 100644 --- a/packages/astro/e2e/error-sass.test.js +++ b/packages/astro/e2e/error-sass.test.js @@ -1,7 +1,10 @@ import { expect } from '@playwright/test'; -import { testFactory, getErrorOverlayMessage } from './test-utils.js'; +import { testFactory, getErrorOverlayContent } from './test-utils.js'; -const test = testFactory({ root: './fixtures/error-sass/' }); +const test = testFactory({ + experimentalErrorOverlay: true, + root: './fixtures/error-sass/' +}); let devServer; @@ -18,7 +21,7 @@ test.describe('Error: Sass', () => { test('overlay', async ({ page, astro }) => { await page.goto(astro.resolveUrl('/')); - const message = await getErrorOverlayMessage(page); + const message = (await getErrorOverlayContent(page)).message; expect(message).toMatch('Undefined variable'); }); }); diff --git a/packages/astro/e2e/errors.test.js b/packages/astro/e2e/errors.test.js index c9aff52699a9..34f3c59ad5e2 100644 --- a/packages/astro/e2e/errors.test.js +++ b/packages/astro/e2e/errors.test.js @@ -1,7 +1,10 @@ import { expect } from '@playwright/test'; -import { getErrorOverlayMessage, testFactory } from './test-utils.js'; +import { getErrorOverlayContent, testFactory } from './test-utils.js'; -const test = testFactory({ root: './fixtures/errors/' }); +const test = testFactory({ + experimentalErrorOverlay: true, + root: './fixtures/errors/' +}); let devServer; @@ -18,7 +21,7 @@ test.describe('Error display', () => { test('detect syntax errors in template', async ({ page, astro }) => { await page.goto(astro.resolveUrl('/astro-syntax-error')); - const message = await getErrorOverlayMessage(page); + const message = (await getErrorOverlayContent(page)).message; expect(message).toMatch('Unexpected "}"'); await Promise.all([ @@ -37,10 +40,8 @@ test.describe('Error display', () => { test('shows useful error when frontmatter import is not found', async ({ page, astro }) => { await page.goto(astro.resolveUrl('/import-not-found')); - const message = await getErrorOverlayMessage(page); - expect(message).toMatch( - 'Could not import `../abc.astro`.\n\nThis is often caused by a typo in the import path. Please make sure the file exists.' - ); + const message = (await getErrorOverlayContent(page)).message; + expect(message).toMatch('Could not import ../abc.astro'); await Promise.all([ // Wait for page reload @@ -55,7 +56,7 @@ test.describe('Error display', () => { test('framework errors recover when fixed', async ({ page, astro }) => { await page.goto(astro.resolveUrl('/svelte-syntax-error')); - const message = await getErrorOverlayMessage(page); + const message = (await getErrorOverlayContent(page)).message; expect(message).toMatch(' attempted to close an element that was not open'); await Promise.all([ diff --git a/packages/astro/e2e/shared-component-tests.js b/packages/astro/e2e/shared-component-tests.js index db8620835883..92423c1c06ec 100644 --- a/packages/astro/e2e/shared-component-tests.js +++ b/packages/astro/e2e/shared-component-tests.js @@ -1,5 +1,5 @@ import { expect } from '@playwright/test'; -import { testFactory, getErrorOverlayMessage } from './test-utils.js'; +import { testFactory } from './test-utils.js'; export function prepareTestFactory(opts) { const test = testFactory(opts); diff --git a/packages/astro/e2e/test-utils.js b/packages/astro/e2e/test-utils.js index 2a8651fd5d9a..b75efeec41da 100644 --- a/packages/astro/e2e/test-utils.js +++ b/packages/astro/e2e/test-utils.js @@ -32,7 +32,12 @@ export function testFactory(inlineConfig) { return test; } -export async function getErrorOverlayMessage(page) { +/** + * + * @param {string} page + * @returns {Promise<{message: string, hint: string}>} + */ +export async function getErrorOverlayContent(page) { const overlay = await page.waitForSelector('vite-error-overlay', { strict: true, timeout: 10 * 1000, @@ -40,7 +45,10 @@ export async function getErrorOverlayMessage(page) { expect(overlay).toBeTruthy(); - return await overlay.$$eval('.message-body', (m) => m[0].textContent); + const message = await overlay.$$eval('#message-content', (m) => m[0].textContent); + const hint = await overlay.$$eval('#hint-content', (m) => m[0].textContent); + + return { message, hint }; } /** diff --git a/packages/astro/src/@types/astro.ts b/packages/astro/src/@types/astro.ts index 1b6c98ea599e..55620c7454f3 100644 --- a/packages/astro/src/@types/astro.ts +++ b/packages/astro/src/@types/astro.ts @@ -82,6 +82,7 @@ export interface CLIFlags { port?: number; config?: string; drafts?: boolean; + experimentalErrorOverlay?: boolean; } export interface BuildConfig { @@ -894,6 +895,12 @@ export interface AstroUserConfig { astroFlavoredMarkdown?: boolean; }; + /** + * @hidden + * Turn on experimental support for the new error overlay component. + */ + experimentalErrorOverlay?: boolean; + // Legacy options to be removed /** @deprecated - Use "integrations" instead. Run Astro to learn more about migrating. */ diff --git a/packages/astro/src/core/compile/style.ts b/packages/astro/src/core/compile/style.ts index 9f32ad3a683e..b9a5bbcfaa1a 100644 --- a/packages/astro/src/core/compile/style.ts +++ b/packages/astro/src/core/compile/style.ts @@ -62,6 +62,7 @@ function enhanceCSSError(err: any, filename: string) { line: errorLine, column: err.column, }, + stack: err.stack, }); } @@ -78,6 +79,7 @@ function enhanceCSSError(err: any, filename: string) { column: err.column, }, frame: err.frame, + stack: err.stack, }); } @@ -94,5 +96,6 @@ function enhanceCSSError(err: any, filename: string) { column: 0, }, frame: err.frame, + stack: err.stack, }); } diff --git a/packages/astro/src/core/config/config.ts b/packages/astro/src/core/config/config.ts index 25efc0306e53..864a13eafc30 100644 --- a/packages/astro/src/core/config/config.ts +++ b/packages/astro/src/core/config/config.ts @@ -100,6 +100,7 @@ export function resolveFlags(flags: Partial): CLIFlags { host: typeof flags.host === 'string' || typeof flags.host === 'boolean' ? flags.host : undefined, drafts: typeof flags.drafts === 'boolean' ? flags.drafts : undefined, + experimentalErrorOverlay: typeof flags.experimentalErrorOverlay === 'boolean' ? flags.experimentalErrorOverlay : undefined, }; } @@ -127,6 +128,7 @@ function mergeCLIFlags(astroConfig: AstroUserConfig, flags: CLIFlags, cmd: strin // TODO: Come back here and refactor to remove this expected error. astroConfig.server.host = flags.host; } + astroConfig.experimentalErrorOverlay = flags.experimentalErrorOverlay ?? false; return astroConfig; } diff --git a/packages/astro/src/core/config/schema.ts b/packages/astro/src/core/config/schema.ts index 9e5fc377b219..78a0ed60c0ed 100644 --- a/packages/astro/src/core/config/schema.ts +++ b/packages/astro/src/core/config/schema.ts @@ -47,6 +47,7 @@ const ASTRO_CONFIG_DEFAULTS: AstroUserConfig & any = { legacy: { astroFlavoredMarkdown: false, }, + experimentalErrorOverlay: false, }; export const AstroConfigSchema = z.object({ @@ -196,6 +197,7 @@ export const AstroConfigSchema = z.object({ }) .optional() .default({}), + experimentalErrorOverlay: z.boolean().optional().default(false), }); interface PostCSSConfigResult { diff --git a/packages/astro/src/core/errors/dev/utils.ts b/packages/astro/src/core/errors/dev/utils.ts index 7ee90115c5a6..9843f3b0ff26 100644 --- a/packages/astro/src/core/errors/dev/utils.ts +++ b/packages/astro/src/core/errors/dev/utils.ts @@ -1,12 +1,15 @@ import * as fs from 'node:fs'; -import { join } from 'node:path'; +import { isAbsolute, join } from 'node:path'; import { fileURLToPath } from 'node:url'; import stripAnsi from 'strip-ansi'; +import { escape } from 'html-escaper'; +import type { BuildResult } from 'esbuild'; import type { ESBuildTransformResult } from 'vite'; import type { SSRError } from '../../../@types/astro.js'; import { AggregateError, ErrorWithMetadata } from '../errors.js'; import { codeFrame } from '../printer.js'; import { normalizeLF } from '../utils.js'; +import { bold, underline } from 'kleur/colors'; type EsbuildMessage = ESBuildTransformResult['warnings'][number]; @@ -30,16 +33,32 @@ export function collectErrorMetadata(e: any, rootFolder?: URL | undefined): Erro error = collectInfoFromStacktrace(e); } - if (error.loc?.file && rootFolder && !error.loc.file.startsWith('/')) { + // Make sure the file location is absolute, otherwise: + // - It won't be clickable in the terminal + // - We'll fail to show the file's content in the browser + // - We'll fail to show the code frame in the terminal + // - The "Open in Editor" button won't work + if ( + error.loc?.file && + rootFolder && + (!error.loc.file.startsWith(rootFolder.pathname) || !isAbsolute(error.loc.file)) + ) { error.loc.file = join(fileURLToPath(rootFolder), error.loc.file); } // If we don't have a frame, but we have a location let's try making up a frame for it - if (!error.frame && error.loc) { + if (error.loc && (!error.frame || !error.fullCode)) { try { const fileContents = fs.readFileSync(error.loc.file!, 'utf8'); - const frame = codeFrame(fileContents, error.loc); - error.frame = frame; + + if (!error.frame) { + const frame = codeFrame(fileContents, error.loc); + error.frame = frame; + } + + if (!error.fullCode) { + error.fullCode = fileContents; + } } catch {} } @@ -47,13 +66,16 @@ export function collectErrorMetadata(e: any, rootFolder?: URL | undefined): Erro error.hint = generateHint(e); }); - // If we received an array of errors and it's not from us, it should be from ESBuild, try to extract info for Vite to display + // If we received an array of errors and it's not from us, it's most likely from ESBuild, try to extract info for Vite to display + // NOTE: We still need to be defensive here, because it might not necessarily be from ESBuild, it's just fairly likely. if (!AggregateError.is(e) && Array.isArray((e as any).errors)) { (e.errors as EsbuildMessage[]).forEach((buildError, i) => { const { location, pluginName, text } = buildError; // ESBuild can give us a slightly better error message than the one in the error, so let's use it - err[i].message = text; + if (text) { + err[i].message = text; + } if (location) { err[i].loc = { file: location.file, line: location.line, column: location.column }; @@ -71,13 +93,17 @@ export function collectErrorMetadata(e: any, rootFolder?: URL | undefined): Erro } } - const possibleFilePath = err[i].pluginCode || err[i].id || location?.file; - if (possibleFilePath && !err[i].frame) { + const possibleFilePath = location?.file ?? err[i].id; + if (possibleFilePath && err[i].loc && (!err[i].frame || !err[i].fullCode)) { try { const fileContents = fs.readFileSync(possibleFilePath, 'utf8'); - err[i].frame = codeFrame(fileContents, { ...err[i].loc, file: possibleFilePath }); + if (!err[i].frame) { + err[i].frame = codeFrame(fileContents, { ...err[i].loc, file: possibleFilePath }); + } + + err[i].fullCode = fileContents; } catch { - // do nothing, code frame isn't that big a deal + err[i].fullCode = err[i].pluginCode; } } @@ -94,15 +120,17 @@ export function collectErrorMetadata(e: any, rootFolder?: URL | undefined): Erro } function generateHint(err: ErrorWithMetadata): string | undefined { + const commonBrowserAPIs = ['document', 'window']; + if (/Unknown file extension \"\.(jsx|vue|svelte|astro|css)\" for /.test(err.message)) { return 'You likely need to add this package to `vite.ssr.noExternal` in your astro config file.'; - } else if (err.toString().includes('document')) { + } else if (commonBrowserAPIs.some((api) => err.toString().includes(api))) { const hint = `Browser APIs are not available on the server. ${ err.loc?.file?.endsWith('.astro') - ? 'Move your code to a `); res.end();