feat(wrangler): improve script source display on pretty error screen (#9971)

* feat(wrangler): improve script source display on pretty error screen

* avoid relying on fs in the patch with style refined

* drop native module script source support

Author: edmundhung

.changeset/good-friends-end.md

@@ -0,0 +1,5 @@
+---
+"wrangler": minor
+---
+
+Improved script source display on the pretty error screen

.changeset/neat-comics-guess.md

@@ -0,0 +1,5 @@
+---
+"miniflare": minor
+---
+
+feat: add stripDisablePrettyError option to control whether the header is stripped

package.json

@@ -84,7 +84,8 @@
 			"[email protected]": "patches/[email protected]",
 			"[email protected]": "patches/[email protected]",
 			"postal-mime": "patches/postal-mime.patch",
-			"@changesets/assemble-release-plan": "patches/@changesets__assemble-release-plan.patch"
+			"@changesets/assemble-release-plan": "patches/@changesets__assemble-release-plan.patch",
+			"[email protected]": "patches/[email protected]"
 		}
 	}
 }

packages/miniflare/src/plugins/core/errors/index.ts

@@ -11,7 +11,7 @@ import {
 	SourceOptions,
 } from "../modules";
 import { getSourceMapper } from "./sourcemap";
-import type { UrlAndMap } from "@cspotcode/source-map-support";
+import type { RawSourceMap, UrlAndMap } from "@cspotcode/source-map-support";
 
 // Subset of core worker options that define Worker source code.
 // These are the possible cases, and corresponding reported source files in
@@ -139,7 +139,11 @@ function getHeaders(request: Request) {
 
 function getSourceMappedStack(
 	workerSrcOpts: NameSourceOptions[],
-	error: Error
+	error: Error,
+	onSourceMap?: (
+		sourcemap: RawSourceMap,
+		sourceMapPath: string
+	) => RawSourceMap | void
 ) {
 	// This function needs to match the signature of the `retrieveSourceMap`
 	// option from the "source-map-support" package.
@@ -160,6 +164,22 @@ function getSourceMappedStack(
 		const sourceMapFile = maybeGetDiskFile(sourceMapPath);
 		if (sourceMapFile === undefined) return null;
 
+		if (onSourceMap) {
+			try {
+				const sourcemap = JSON.parse(sourceMapFile.contents);
+				const result = onSourceMap(sourcemap, sourceMapFile.path);
+
+				if (result !== undefined) {
+					return {
+						map: JSON.stringify(result),
+						url: sourceMapFile.path,
+					};
+				}
+			} catch {
+				return null;
+			}
+		}
+
 		return { map: sourceMapFile.contents, url: sourceMapFile.path };
 	}
 
@@ -201,14 +221,18 @@ const ALLOWED_ERROR_SUBCLASS_CONSTRUCTORS: StandardErrorConstructor[] = [
 ];
 export function reviveError(
 	workerSrcOpts: NameSourceOptions[],
-	jsonError: JsonError
+	jsonError: JsonError,
+	onSourceMap?: (
+		sourcemap: RawSourceMap,
+		sourceMapPath: string
+	) => RawSourceMap | void
 ): Error {
 	// At a high level, this function takes a JSON-serialisable representation of
 	// an `Error`, and converts it to an `Error`. `Error`s may have `cause`s, so
 	// we need to do this recursively.
 	let cause: Error | undefined;
 	if (jsonError.cause !== undefined) {
-		cause = reviveError(workerSrcOpts, jsonError.cause);
+		cause = reviveError(workerSrcOpts, jsonError.cause, onSourceMap);
 	}
 
 	// If this is one of the built-in error types, construct an instance of that.
@@ -236,7 +260,7 @@ export function reviveError(
 
 	// Try to apply source-mapping to the stack trace
 	error.stack = processStackTrace(
-		getSourceMappedStack(workerSrcOpts, error),
+		getSourceMappedStack(workerSrcOpts, error, onSourceMap),
 		(line, location) =>
 			!location.includes(".wrangler/tmp") &&
 			!location.includes("wrangler/templates/middleware")
@@ -255,11 +279,28 @@ export async function handlePrettyErrorRequest(
 	// Parse and validate the error we've been given from user code
 	const caught = JsonErrorSchema.parse(await request.json());
 
-	// Convert the error into a regular `Error` object and try to source-map it.
-	// We need to give `name`, `message` and `stack` to Youch, but StackTracy,
-	// Youch's dependency for parsing `stack`s, will only extract `stack` from
-	// an object if it's an `instanceof Error`.
-	const error = reviveError(workerSrcOpts, caught);
+	// We might populate the source in Youch with the sources content from the sourcemap
+	// in case the source file does not exist on disk
+	const sourcesContentMap = new Map<string, string>();
+	const error = reviveError(
+		workerSrcOpts,
+		caught,
+		(sourceMap, sourceMapPath) => {
+			for (let i = 0; i < sourceMap.sources.length; i++) {
+				const source = sourceMap.sources[i];
+				const sourceContent = sourceMap.sourcesContent?.[i];
+
+				if (sourceContent) {
+					const fullSourcePath = path.resolve(
+						path.dirname(sourceMapPath),
+						sourceMap.sourceRoot ?? "",
+						source
+					);
+					sourcesContentMap.set(fullSourcePath, sourceContent);
+				}
+			}
+		}
+	);
 
 	// Log source-mapped error to console if logging enabled
 	log.error(error);
@@ -281,10 +322,53 @@ export async function handlePrettyErrorRequest(
 	// Lazily import `youch` when required
 	// eslint-disable-next-line @typescript-eslint/no-require-imports
 	const { Youch }: typeof import("youch") = require("youch");
-	// `cause` is usually more useful than the error itself, display that instead
-	// TODO(someday): would be nice if we could display both
 	const youch = new Youch();
 
+	youch.defineSourceLoader(async (stackFrame) => {
+		if (!stackFrame.fileName) {
+			return;
+		}
+
+		if (
+			stackFrame.type === "native" ||
+			stackFrame.fileName.startsWith("cloudflare:") ||
+			stackFrame.fileName.startsWith("node:")
+		) {
+			stackFrame.type = "native";
+			return;
+		}
+
+		if (!fs.existsSync(stackFrame.fileName)) {
+			// Mark the file type as undefined to skip the link to the source file
+			stackFrame.fileType = undefined;
+		}
+
+		// Check if it is an inline script, which are reported as "script-<workerIndex>"
+		const workerIndex = maybeGetStringScriptPathIndex(stackFrame.fileName);
+		if (workerIndex !== undefined) {
+			const srcOpts = workerSrcOpts[workerIndex];
+			if ("script" in srcOpts && srcOpts.script !== undefined) {
+				return { contents: srcOpts.script };
+			}
+		}
+
+		// Check if the source map has the source content for this file so we don't need to read it from disk
+		const sourcesContent = sourcesContentMap.get(stackFrame.fileName);
+		if (sourcesContent) {
+			return { contents: sourcesContent };
+		}
+
+		try {
+			const contents = fs.readFileSync(stackFrame.fileName, {
+				encoding: "utf8",
+			});
+
+			return { contents };
+		} catch {
+			// If we can't read the file, just return undefined
+			return;
+		}
+	});
 	youch.useTransformer((error) => {
 		error.hint = [
 			'<a href="https://developers.cloudflare.com/workers/" target="_blank" style="text-decoration:none;font-style:normal;padding:5px">📚 Workers Docs</a>',

packages/miniflare/src/plugins/core/index.ts

@@ -245,6 +245,8 @@ export const CoreSharedOptionsSchema = z.object({
 	// Path to the root directory for persisting data
 	// Used as the default for all plugins with the plugin name as the subdirectory name
 	defaultPersistRoot: z.string().optional(),
+	// Strip the MF-DISABLE_PRETTY_ERROR header from user request
+	stripDisablePrettyError: z.boolean().default(true),
 });
 
 export const CORE_PLUGIN_NAME = "core";
@@ -917,6 +919,10 @@ export function getGlobalServices({
 			name: CoreBindings.DATA_PROXY_SECRET,
 			data: PROXY_SECRET,
 		},
+		{
+			name: CoreBindings.STRIP_DISABLE_PRETTY_ERROR,
+			json: JSON.stringify(sharedOptions.stripDisablePrettyError),
+		},
 		// Add `proxyBindings` here, they'll be added to the `ProxyServer` `env`
 		...proxyBindings,
 	];

packages/miniflare/src/workers/core/constants.ts

@@ -34,6 +34,7 @@ export const CoreBindings = {
 	DATA_PROXY_SHARED_SECRET: "MINIFLARE_PROXY_SHARED_SECRET",
 	TRIGGER_HANDLERS: "TRIGGER_HANDLERS",
 	LOG_REQUESTS: "LOG_REQUESTS",
+	STRIP_DISABLE_PRETTY_ERROR: "STRIP_DISABLE_PRETTY_ERROR",
 } as const;
 
 export const ProxyOps = {

packages/miniflare/src/workers/core/entry.worker.ts

@@ -29,6 +29,7 @@ type Env = {
 	[CoreBindings.DATA_PROXY_SHARED_SECRET]?: ArrayBuffer;
 	[CoreBindings.TRIGGER_HANDLERS]: boolean;
 	[CoreBindings.LOG_REQUESTS]: boolean;
+	[CoreBindings.STRIP_DISABLE_PRETTY_ERROR]: boolean;
 } & {
 	[K in `${typeof CoreBindings.SERVICE_USER_ROUTE_PREFIX}${string}`]:
 		| Fetcher
@@ -124,7 +125,9 @@ function getUserRequest(
 
 	request.headers.delete(CoreHeaders.PROXY_SHARED_SECRET);
 	request.headers.delete(CoreHeaders.ORIGINAL_URL);
-	request.headers.delete(CoreHeaders.DISABLE_PRETTY_ERROR);
+	if (env[CoreBindings.STRIP_DISABLE_PRETTY_ERROR]) {
+		request.headers.delete(CoreHeaders.DISABLE_PRETTY_ERROR);
+	}
 	return request;
 }
 

packages/miniflare/test/plugins/core/errors/index.spec.ts

@@ -305,20 +305,31 @@ test("responds with pretty error page", async (t) => {
 		log,
 		modules: true,
 		script: `
-		function doSomething() {
+		import { connect } from "cloudflare:sockets";
+
+		// A function to test error thrown by native code
+		async function connectSocket(request) {
 			try {
-				oops();
+				// The following line will throw an error because the port is invalid
+				const socket = connect({ hostname: "gopher.floodgap.com", port: "invalid" });
+
+				const writer = socket.writable.getWriter();
+				const url = new URL(request.url);
+				const encoder = new TextEncoder();
+				const encoded = encoder.encode(url.pathname + "\\r\\n");
+				await writer.write(encoded);
+				await writer.close();
+
+				return new Response(socket.readable, {
+					headers: { "Content-Type": "text/plain" },
+				});
 			} catch (e) {
-			 	throw new Error("Unusual oops!", {
+				throw new Error("Unusual oops!", {
 					cause: e,
-				})
+				});
 			}
 		}
 
-        function oops() {
-		  throw new Error("Test error");
-		}
-
 		// This emulates the reduceError function in the Wrangler middleware template
 		// See packages/wrangler/templates/middleware/middleware-miniflare3-json-error.ts
 		function reduceError(e) {
@@ -331,17 +342,17 @@ test("responds with pretty error page", async (t) => {
 		}
 
 		export default {
-			async fetch() {
+			async fetch(request) {
 				try {
-					doSomething();
+					return await connectSocket(request);
 				} catch (e) {
 					const error = reduceError(e);
 					return Response.json(error, {
 						status: 500,
 						headers: { "MF-Experimental-Error-Stack": "true" },
 					});
 				}
-			}
+			},
 		}`,
 	});
 	t.teardown(() => mf.dispose());
@@ -360,17 +371,22 @@ test("responds with pretty error page", async (t) => {
 	t.regex(text, /Method.+POST/is);
 	t.regex(text, /URL.+some-unusual-path/is);
 	t.regex(text, /X-Unusual-Key.+some-unusual-value/is);
+	// Check if the stack trace is included
+	t.regex(text, /cloudflare\:sockets/);
+	t.regex(text, /connectSocket/);
+	t.regex(text, /connect/);
+	t.regex(text, /Object\.fetch/);
 
 	// Check error logged
 	const errorLogs = log.getLogs(LogLevel.ERROR);
 	t.deepEqual(errorLogs, [
 		`Error: Unusual oops!
-    at doSomething (script-0:6:12)
-    at Object.fetch (script-0:30:6)
-Caused by: Error: Test error
-    at oops (script-0:13:11)
-    at doSomething (script-0:4:5)
-    at Object.fetch (script-0:30:6)`,
+    at connectSocket (script-0:21:11)
+    at Object.fetch (script-0:41:19)
+Caused by: TypeError: The value cannot be converted because it is not an integer.
+    at connect (cloudflare:sockets:7:20)
+    at connectSocket (script-0:8:20)
+    at Object.fetch (script-0:41:19)`,
 	]);
 
 	// Check `fetch()` accepting HTML returns pretty-error page

packages/wrangler/src/api/startDevWorker/ProxyController.ts

@@ -86,7 +86,7 @@ export class ProxyController extends Controller<ProxyControllerEventMap> {
 			https: this.latestConfig.dev?.server?.secure,
 			httpsCert: cert?.cert,
 			httpsKey: cert?.key,
-
+			stripDisablePrettyError: false,
 			workers: [
 				{
 					name: "ProxyWorker",

packages/wrangler/templates/startDevWorker/ProxyWorker.ts

@@ -129,7 +129,6 @@ export class ProxyWorker implements DurableObject {
 				request.url
 			);
 			headers.set("MF-Original-URL", innerUrl.href);
-			headers.set("MF-Disable-Pretty-Error", "true"); // disables the UserWorker miniflare instance from rendering the pretty error -- instead the ProxyWorker miniflare instance will intercept the json error response and render the pretty error page
 
 			// Preserve client `Accept-Encoding`, rather than using Worker's default
 			// of `Accept-Encoding: br, gzip`