Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect path in error overlay on Win #6679

Merged
merged 1 commit into from
Mar 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/honest-paws-behave.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Fix incorrect path to file in error overlay on Win
22 changes: 22 additions & 0 deletions packages/astro/e2e/errors.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,28 @@ test.describe('Error display', () => {
expect(await page.locator('vite-error-overlay').count()).toEqual(0);
});

test('shows correct file path when a page has an error', async ({ page, astro }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two different tests because on Windows the path separators are different depending on the location of the error (i.e. component vs page).

await page.goto(astro.resolveUrl('/import-not-found'));

const { fileLocation, absoluteFileLocation } = await getErrorOverlayContent(page);
const absoluteFileUrl = 'file://' + absoluteFileLocation.replace(/:\d+:\d+$/, '');
const fileExists = astro.pathExists(absoluteFileUrl);

expect(fileExists).toBeTruthy();
expect(fileLocation).toMatch(/^pages\/import-not-found\.astro/);
});

test('shows correct file path when a component has an error', async ({ page, astro }) => {
await page.goto(astro.resolveUrl('/preact-runtime-error'));

const { fileLocation, absoluteFileLocation } = await getErrorOverlayContent(page);
const absoluteFileUrl = 'file://' + absoluteFileLocation.replace(/:\d+:\d+$/, '');
const fileExists = astro.pathExists(absoluteFileUrl);

expect(fileExists).toBeTruthy();
expect(fileLocation).toMatch(/^components\/PreactRuntimeError.jsx/);
});

test('framework errors recover when fixed', async ({ page, astro }) => {
await page.goto(astro.resolveUrl('/svelte-syntax-error'));

Expand Down
9 changes: 6 additions & 3 deletions packages/astro/e2e/test-utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export function testFactory(inlineConfig) {
/**
*
* @param {string} page
* @returns {Promise<{message: string, hint: string}>}
* @returns {Promise<{message: string, hint: string, absoluteFileLocation: string, fileLocation: string}>}
*/
export async function getErrorOverlayContent(page) {
const overlay = await page.waitForSelector('vite-error-overlay', {
Expand All @@ -47,8 +47,11 @@ export async function getErrorOverlayContent(page) {

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 };
const [absoluteFileLocation, fileLocation] = await overlay.$$eval('#code header h2', (m) => [
m[0].title,
m[0].textContent,
]);
return { message, hint, absoluteFileLocation, fileLocation };
}

/**
Expand Down
9 changes: 8 additions & 1 deletion packages/astro/src/core/errors/dev/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@ import * as fs from 'node:fs';
import { isAbsolute, join } from 'node:path';
import { fileURLToPath } from 'node:url';
import stripAnsi from 'strip-ansi';
import { normalizePath } from 'vite';
import type { ESBuildTransformResult } from 'vite';
import type { SSRError } from '../../../@types/astro.js';
import { AggregateError, type ErrorWithMetadata } from '../errors.js';
import { codeFrame } from '../printer.js';
import { normalizeLF } from '../utils.js';
import { removeLeadingForwardSlashWindows } from '../../path.js';

type EsbuildMessage = ESBuildTransformResult['warnings'][number];

Expand All @@ -32,10 +34,15 @@ export function collectErrorMetadata(e: any, rootFolder?: URL | undefined): Erro
// - 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

// Normalize the paths so that we can correctly detect if it's absolute on any platform
const normalizedFile = normalizePath(error.loc?.file || '');
const normalizedRootFolder = removeLeadingForwardSlashWindows(rootFolder?.pathname || '');
Copy link
Contributor Author

@fcFn fcFn Mar 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove a leading forward slash which rootFolder.pathname comes with which is invalid on Windows (e.g. /C:/) and will always fail the check for the path being absolute below which results in the bug that is described in the issue.


if (
error.loc?.file &&
rootFolder &&
(!error.loc.file.startsWith(rootFolder.pathname) || !isAbsolute(error.loc.file))
(!normalizedFile?.startsWith(normalizedRootFolder) || !isAbsolute(normalizedFile))
) {
error.loc.file = join(fileURLToPath(rootFolder), error.loc.file);
}
Expand Down
3 changes: 2 additions & 1 deletion packages/astro/src/core/errors/overlay.ts
Original file line number Diff line number Diff line change
Expand Up @@ -637,7 +637,8 @@ class ErrorOverlay extends HTMLElement {
const codeContent = code.querySelector<HTMLDivElement>('#code-content');

if (codeHeader) {
const cleanFile = err.loc.file.split('/').slice(-2).join('/');
const separator = err.loc.file.includes('/') ? '/' : '\\';
Copy link
Contributor Author

@fcFn fcFn Mar 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Account for Windows separators

const cleanFile = err.loc.file.split(separator).slice(-2).join('/');
const fileLocation = [cleanFile, err.loc.line, err.loc.column].filter(Boolean).join(':');
const absoluteFileLocation = [err.loc.file, err.loc.line, err.loc.column]
.filter(Boolean)
Expand Down