From 8e26bea8bcbc49a794c623472f9905d53d3c5872 Mon Sep 17 00:00:00 2001 From: Caleb Eby Date: Mon, 9 Aug 2021 13:46:58 -0700 Subject: [PATCH] Make loadJS use the same error handling as runJS (#199) --- .changeset/early-rules-knock.md | 5 ++ src/index.ts | 108 +++++---------------------- src/source-map-error-from-browser.ts | 100 +++++++++++++++++++++++++ tests/test-utils.ts | 25 +++++++ tests/utils/external-throwing.ts | 3 + tests/utils/loadJS.test.ts | 41 +++++++++- tests/utils/runJS.test.tsx | 53 ++++--------- 7 files changed, 205 insertions(+), 130 deletions(-) create mode 100644 .changeset/early-rules-knock.md create mode 100644 src/source-map-error-from-browser.ts create mode 100644 tests/utils/external-throwing.ts diff --git a/.changeset/early-rules-knock.md b/.changeset/early-rules-knock.md new file mode 100644 index 00000000..2cfb5532 --- /dev/null +++ b/.changeset/early-rules-knock.md @@ -0,0 +1,5 @@ +--- +'pleasantest': minor +--- + +Make loadJS share error mapping logic with runJS diff --git a/src/index.ts b/src/index.ts index c68d4a58..779851fd 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,5 +1,5 @@ import * as puppeteer from 'puppeteer'; -import { relative, join, isAbsolute, dirname, posix, sep, resolve } from 'path'; +import { relative, join, isAbsolute, dirname } from 'path'; import type { BoundQueries } from './pptr-testing-library'; import { getQueriesForElement } from './pptr-testing-library'; import { connectToBrowser } from './connect-to-browser'; @@ -11,16 +11,13 @@ import _ansiRegex from 'ansi-regex'; import { fileURLToPath } from 'url'; import type { PleasantestUser } from './user'; import { pleasantestUser } from './user'; -import { - assertElementHandle, - printStackLine, - removeFuncFromStackTrace, -} from './utils'; +import { assertElementHandle, removeFuncFromStackTrace } from './utils'; import type { ModuleServerOpts } from './module-server'; import { createModuleServer } from './module-server'; import { cleanupClientRuntimeServer } from './module-server/client-runtime-server'; import { Console } from 'console'; import { createBuildStatusTracker } from './module-server/build-status-tracker'; +import { sourceMapErrorFromBrowser } from './source-map-error-from-browser'; export { JSHandle, ElementHandle } from 'puppeteer'; koloristOpts.enabled = true; @@ -320,7 +317,7 @@ const createTab = async ({ // This uses the testPath as the url so that if there are relative imports // in the inline code, the relative imports are resolved relative to the test file const url = `http://localhost:${port}/${testPath}?inline-code=${encodedCode}&build-id=${buildStatus.buildId}`; - const res = (await safeEvaluate( + const res = await safeEvaluate( runJS, new Function( '...args', @@ -334,91 +331,13 @@ const createTab = async ({ : e)`, ) as () => any, ...(Array.isArray(args) ? (args as any) : []), - )) as undefined | { message: string; stack: string }; + ); const errorsFromBuild = buildStatus.complete(); // It only throws the first one but that is probably OK if (errorsFromBuild) throw errorsFromBuild[0]; - if (res === undefined) return; - if (typeof res !== 'object') throw res; - const { message, stack } = res; - const parsedStack = parseStackTrace(stack); - const modifiedStack = parsedStack.map(async (stackItem) => { - if (stackItem.raw.startsWith(stack.slice(0, stack.indexOf('\n')))) - return null; - if (!stackItem.fileName) return stackItem.raw; - const fileName = stackItem.fileName; - const line = stackItem.line; - const column = stackItem.column; - if (!fileName.startsWith(`http://localhost:${port}`)) - return stackItem.raw; - const url = new URL(fileName); - const osPath = url.pathname.slice(1).split(posix.sep).join(sep); - // Absolute file path - const file = resolve(process.cwd(), osPath); - // Rollup-style Unix-normalized path "id": - const id = file.split(sep).join(posix.sep); - const transformResult = requestCache.get(id); - const map = typeof transformResult === 'object' && transformResult.map; - if (!map) { - let p = url.pathname; - const npmPrefix = '/@npm/'; - if (p.startsWith(npmPrefix)) - p = join(process.cwd(), 'node_modules', p.slice(npmPrefix.length)); - return printStackLine(p, line, column, stackItem.name); - } - - const { SourceMapConsumer } = await import('source-map'); - const consumer = await new SourceMapConsumer(map as any); - const sourceLocation = consumer.originalPositionFor({ - line, - column: column - 1, // Source-map uses zero-based column numbers - }); - consumer.destroy(); - return printStackLine( - join(process.cwd(), url.pathname), - sourceLocation.line ?? line, - sourceLocation.column === null - ? column - : // Convert back from zero-based column to 1-based - sourceLocation.column + 1, - stackItem.name, - ); - }); - const errorName = stack.slice(0, stack.indexOf(':')) || 'Error'; - const specializedErrors = { - EvalError, - RangeError, - ReferenceError, - SyntaxError, - TypeError, - URIError, - } as any; - const ErrorConstructor: ErrorConstructor = - specializedErrors[errorName] || Error; - const error = new ErrorConstructor(message); - - const finalStack = (await Promise.all(modifiedStack)) - .filter(Boolean) - .join('\n'); - - // If the browser error did not provide a stack, use the stack trace from node - if (finalStack) { - error.stack = `${errorName}: ${message}\n${finalStack}`; - } else { - removeFuncFromStackTrace(error, runJS); - if (error.stack) - error.stack = error.stack - .split('\n') - .filter( - // This was appearing in stack traces and it messed up the Jest output - (line) => !(/runMicrotasks/.test(line) && //.test(line)), - ) - .join('\n'); - } - - throw error; + await sourceMapErrorFromBrowser(res, requestCache, port, runJS); }; const injectHTML: PleasantestUtils['injectHTML'] = async (html) => { @@ -459,10 +378,21 @@ const createTab = async ({ const fullPath = jsPath.startsWith('.') ? join(dirname(testPath), jsPath) : jsPath; - await safeEvaluate( + const buildStatus = createBuildStatusTracker(); + const url = `http://localhost:${port}/${fullPath}?build-id=${buildStatus.buildId}`; + const res = await safeEvaluate( loadJS, - `import(${JSON.stringify(`http://localhost:${port}/${fullPath}`)})`, + `import(${JSON.stringify(url)}) + .catch(e => e instanceof Error + ? { message: e.message, stack: e.stack } + : e)`, ); + + const errorsFromBuild = buildStatus.complete(); + // It only throws the first one but that is probably OK + if (errorsFromBuild) throw errorsFromBuild[0]; + + await sourceMapErrorFromBrowser(res, requestCache, port, loadJS); }; const utils: PleasantestUtils = { diff --git a/src/source-map-error-from-browser.ts b/src/source-map-error-from-browser.ts new file mode 100644 index 00000000..37d9b98a --- /dev/null +++ b/src/source-map-error-from-browser.ts @@ -0,0 +1,100 @@ +import { join, posix, sep, resolve } from 'path'; +import { parseStackTrace } from 'errorstacks'; +import _ansiRegex from 'ansi-regex'; +import { printStackLine, removeFuncFromStackTrace } from './utils'; +import type { SourceDescription } from 'rollup'; + +export const sourceMapErrorFromBrowser = async ( + res: unknown, + requestCache: Map, + port: number, + func: (...args: any[]) => void, +) => { + if (res === undefined) return; + if ( + typeof res !== 'object' || + !res || + !('message' in res) || + !('stack' in res) + ) + throw res; + const { message, stack } = res as { + message: string; + stack: string; + }; + const parsedStack = parseStackTrace(stack); + const modifiedStack = parsedStack.map(async (stackItem) => { + if (stackItem.raw.startsWith(stack.slice(0, stack.indexOf('\n')))) + return null; + if (!stackItem.fileName) return stackItem.raw; + const fileName = stackItem.fileName; + const line = stackItem.line; + const column = stackItem.column; + if (!fileName.startsWith(`http://localhost:${port}`)) return stackItem.raw; + const url = new URL(fileName); + const osPath = url.pathname.slice(1).split(posix.sep).join(sep); + // Absolute file path + const file = resolve(process.cwd(), osPath); + // Rollup-style Unix-normalized path "id": + const id = file.split(sep).join(posix.sep); + const transformResult = requestCache.get(id); + const map = typeof transformResult === 'object' && transformResult.map; + if (!map) { + let p = url.pathname; + const npmPrefix = '/@npm/'; + if (p.startsWith(npmPrefix)) + p = join(process.cwd(), 'node_modules', p.slice(npmPrefix.length)); + return printStackLine(p, line, column, stackItem.name); + } + + const { SourceMapConsumer } = await import('source-map'); + const consumer = await new SourceMapConsumer(map as any); + const sourceLocation = consumer.originalPositionFor({ + line, + column: column - 1, // Source-map uses zero-based column numbers + }); + consumer.destroy(); + return printStackLine( + join(process.cwd(), url.pathname), + sourceLocation.line ?? line, + sourceLocation.column === null + ? column + : // Convert back from zero-based column to 1-based + sourceLocation.column + 1, + stackItem.name, + ); + }); + const errorName = stack.slice(0, stack.indexOf(':')) || 'Error'; + const ErrorConstructor = specializedErrors[errorName] || Error; + const error = new ErrorConstructor(message); + + const finalStack = (await Promise.all(modifiedStack)) + .filter(Boolean) + .join('\n'); + + // If the browser error did not provide a stack, use the stack trace from node + if (finalStack) { + error.stack = `${errorName}: ${message}\n${finalStack}`; + } else { + removeFuncFromStackTrace(error, func); + if (error.stack) + error.stack = error.stack + .split('\n') + .filter( + // This was appearing in stack traces and it messed up the Jest output + (line) => !(/runMicrotasks/.test(line) && //.test(line)), + ) + .join('\n'); + } + + throw error; +}; + +const specializedErrors: Record = { + EvalError, + RangeError, + ReferenceError, + SyntaxError, + TypeError, + URIError, +}; diff --git a/tests/test-utils.ts b/tests/test-utils.ts index 3be9d645..3df7007b 100644 --- a/tests/test-utils.ts +++ b/tests/test-utils.ts @@ -1,6 +1,7 @@ import { parseStackTrace } from 'errorstacks'; import { promises as fs } from 'fs'; import * as path from 'path'; +import ansiRegex from 'ansi-regex'; export const printErrorFrames = async (error?: Error) => { if (!error?.stack) return ''; @@ -45,3 +46,27 @@ export const printErrorFrames = async (error?: Error) => { `\n${'-'.repeat(55)}\n`, ); }; + +const stripAnsi = (input: string) => input.replace(ansiRegex(), ''); + +const removeLineNumbers = (input: string) => { + const lineRegex = /^(\s*>?\s*)(\d+)/gm; + const fileRegex = new RegExp(`${process.cwd()}([a-zA-Z/._-]*)[\\d:]*`, 'g'); + return ( + input + .replace( + lineRegex, + (_match, whitespace, numbers) => + `${whitespace}${'#'.repeat(numbers.length)}`, + ) + // Take out the file paths so the tests will pass on more than 1 person's machine + .replace(fileRegex, '$1:###:###') + ); +}; + +export const formatErrorWithCodeFrame = (input: Promise) => + input.catch((error) => { + error.message = removeLineNumbers(stripAnsi(error.message)); + error.stack = removeLineNumbers(stripAnsi(error.stack)); + throw error; + }); diff --git a/tests/utils/external-throwing.ts b/tests/utils/external-throwing.ts new file mode 100644 index 00000000..3949e20e --- /dev/null +++ b/tests/utils/external-throwing.ts @@ -0,0 +1,3 @@ +let foo: string; + +throw new Error('asdf'); diff --git a/tests/utils/loadJS.test.ts b/tests/utils/loadJS.test.ts index 9c5af049..011b8e24 100644 --- a/tests/utils/loadJS.test.ts +++ b/tests/utils/loadJS.test.ts @@ -1,3 +1,40 @@ +import { withBrowser } from 'pleasantest'; +import { formatErrorWithCodeFrame, printErrorFrames } from '../test-utils'; + test.todo('loads from .ts file with transpiling'); -test.todo('if the file throws an error the error is source mapped'); -test.todo('if the file has a syntax error the location is source mapped'); + +test( + 'if the file throws an error the error is source mapped', + withBrowser(async ({ utils }) => { + const error = await utils + .loadJS('./external-throwing.ts') + .catch((error) => error); + expect(await printErrorFrames(error)).toMatchInlineSnapshot(` + "Error: asdf + ------------------------------------------------------- + tests/utils/external-throwing.ts + + throw new Error('asdf'); + ^" + `); + }), +); + +test( + 'if the file has a syntax error the location is source mapped', + withBrowser(async ({ utils }) => { + const loadPromise = utils.loadJS('./external-with-syntax-error.ts'); + await expect(formatErrorWithCodeFrame(loadPromise)).rejects + .toThrowErrorMatchingInlineSnapshot(` + "[esbuild] The constant \\"someVariable\\" must be initialized + + /tests/utils/external-with-syntax-error.ts:###:### + + # | // @ts-expect-error: this is intentionally invalid + > # | const someVariable: string; + | ^ + # | + " + `); + }), +); diff --git a/tests/utils/runJS.test.tsx b/tests/utils/runJS.test.tsx index 32824f65..2cd2964f 100644 --- a/tests/utils/runJS.test.tsx +++ b/tests/utils/runJS.test.tsx @@ -1,10 +1,9 @@ import { withBrowser } from 'pleasantest'; import type { PleasantestContext, PleasantestUtils } from 'pleasantest'; -import { printErrorFrames } from '../test-utils'; +import { formatErrorWithCodeFrame, printErrorFrames } from '../test-utils'; import vuePlugin from 'rollup-plugin-vue'; import aliasPlugin from '@rollup/plugin-alias'; import babel from '@rollup/plugin-babel'; -import ansiRegex from 'ansi-regex'; const createHeading = async ({ utils, @@ -176,19 +175,19 @@ test( await expect(formatErrorWithCodeFrame(runPromise)).rejects .toThrowErrorMatchingInlineSnapshot(` - "Error parsing module with es-module-lexer - - /tests/utils/runJS.test.tsx:###:### - - ### | async ({ utils }) => { - ### | const runPromise = utils.runJS(\` - > ### | asdf()) - | ^ - ### | \`); - ### | - ### | await expect(formatErrorWithCodeFrame(runPromise)).rejects - " - `); + "Error parsing module with es-module-lexer + + /tests/utils/runJS.test.tsx:###:### + + ### | async ({ utils }) => { + ### | const runPromise = utils.runJS(\` + > ### | asdf()) + | ^ + ### | \`); + ### | + ### | await expect(formatErrorWithCodeFrame(runPromise)).rejects + " + `); }, ), ); @@ -252,30 +251,6 @@ test( }), ); -const stripAnsi = (input: string) => input.replace(ansiRegex(), ''); - -const removeLineNumbers = (input: string) => { - const lineRegex = /^(\s*>?\s*)(\d+)/gm; - const fileRegex = new RegExp(`${process.cwd()}([a-zA-Z/._-]*)[\\d:]*`, 'g'); - return ( - input - .replace( - lineRegex, - (_match, whitespace, numbers) => - `${whitespace}${'#'.repeat(numbers.length)}`, - ) - // Take out the file paths so the tests will pass on more than 1 person's machine - .replace(fileRegex, '$1:###:###') - ); -}; - -const formatErrorWithCodeFrame = (input: Promise) => - input.catch((error) => { - error.message = removeLineNumbers(stripAnsi(error.message)); - error.stack = removeLineNumbers(stripAnsi(error.stack)); - throw error; - }); - test( 'If the code string has a syntax error the location is source mapped', withBrowser(async ({ utils }) => {