[wrangler] fix: apply source mapping to deployment validation errors (#…

…4600)

* fix: apply source mapping to deployment validation errors

* fixup! fix: apply source mapping to deployment validation errors

Rename `RetrieveSourceMap` to `RetrieveSourceMapFunction`

* fixup! fix: apply source mapping to deployment validation errors

Add comment highlighting why `retrieveSourceMapOverride` module level

* fixup! fix: apply source mapping to deployment validation errors

Add tests for different types of modules/source maps

* fixup! fix: apply source mapping to deployment validation errors

Use `dedent` with multiline template literals

* fixup! fix: apply source mapping to deployment validation errors

Use correct variable name in error

.changeset/olive-chefs-sleep.md

@@ -0,0 +1,7 @@
+---
+"wrangler": patch
+---
+
+fix: apply source mapping to deployment validation errors
+
+Previously if a Worker failed validation during `wrangler deploy`, the displayed error would reference locations in built JavaScript files. This made it more difficult to debug validation errors. This change ensures these errors are now source mapped, referencing locations in source files instead.

packages/wrangler/src/__tests__/deploy.test.ts

@@ -7,6 +7,7 @@ import * as TOML from "@iarna/toml";
 import commandExists from "command-exists";
 import * as esbuild from "esbuild";
 import { MockedRequest, rest } from "msw";
+import dedent from "ts-dedent";
 import { FormData } from "undici";
 import {
 	printBundleSize,
@@ -1835,6 +1836,119 @@ addEventListener('fetch', event => {});`
 			}
 		`);
 		});
+
+		describe("should source map validation errors", () => {
+			function mockDeployWithValidationError(message: string) {
+				const handler = rest.put(
+					"*/accounts/:accountId/workers/scripts/:scriptName",
+					async (req, res, ctx) => {
+						const body = createFetchResult(null, false, [
+							{ code: 10021, message },
+						]);
+						return res(ctx.json(body));
+					}
+				);
+				msw.use(handler);
+			}
+
+			it("with TypeScript source file", async () => {
+				writeWranglerToml();
+				fs.writeFileSync(
+					`index.ts`,
+					dedent`interface Env {
+						THING: string;
+					}
+					x;
+					export default {
+						fetch() {
+							return new Response("body");
+						}
+					}`
+				);
+				mockDeployWithValidationError(
+					"Uncaught ReferenceError: x is not defined\n  at index.js:2:1\n"
+				);
+				mockSubDomainRequest();
+
+				await expect(runWrangler("deploy ./index.ts")).rejects.toMatchObject({
+					notes: [{ text: expect.stringContaining("index.ts:4:1") }, {}],
+				});
+			});
+
+			it("with additional modules", async () => {
+				writeWranglerToml({
+					no_bundle: true,
+					rules: [{ type: "ESModule", globs: ["**/*.js"] }],
+				});
+
+				fs.writeFileSync(
+					"dep.ts",
+					dedent`interface Env {
+					}
+					y;
+					export default "message";`
+				);
+				await esbuild.build({
+					bundle: true,
+					format: "esm",
+					entryPoints: [path.resolve("dep.ts")],
+					outdir: process.cwd(),
+					sourcemap: true,
+				});
+
+				fs.writeFileSync(
+					"index.js",
+					dedent`import dep from "./dep.js";
+					export default {
+						fetch() {
+							return new Response(dep);
+						}
+					}`
+				);
+
+				mockDeployWithValidationError(
+					"Uncaught ReferenceError: y is not defined\n  at dep.js:2:1\n"
+				);
+				mockSubDomainRequest();
+
+				await expect(runWrangler("deploy ./index.js")).rejects.toMatchObject({
+					notes: [{ text: expect.stringContaining("dep.ts:3:1") }, {}],
+				});
+			});
+
+			it("with inline source map", async () => {
+				writeWranglerToml({
+					no_bundle: true,
+				});
+
+				fs.writeFileSync(
+					"index.ts",
+					dedent`interface Env {}
+					z;
+					export default {
+						fetch() {
+							return new Response("body");
+						}
+					}`
+				);
+				await esbuild.build({
+					bundle: true,
+					format: "esm",
+					entryPoints: [path.resolve("index.ts")],
+					outdir: process.cwd(),
+					sourcemap: "inline",
+				});
+
+				mockDeployWithValidationError(
+					"Uncaught ReferenceError: z is not defined\n  at index.js:2:1\n"
+				);
+				mockSubDomainRequest();
+
+				await expect(runWrangler("deploy ./index.js")).rejects.toMatchObject({
+					notes: [{ text: expect.stringContaining("index.ts:2:1") }, {}],
+				});
+			});
+		});
 	});
 
 	describe("asset upload", () => {

packages/wrangler/src/deploy/deploy.ts

@@ -30,6 +30,10 @@ import { getWranglerTmpDir } from "../paths";
 import { getQueue, putConsumer } from "../queues/client";
 import { getWorkersDevSubdomain } from "../routes";
 import { syncAssets } from "../sites";
+import {
+	getSourceMappedString,
+	maybeRetrieveFileSourceMap,
+} from "../sourcemap";
 import { getZoneForRoute } from "../zones";
 import type { FetchError } from "../cfetch";
 import type { Config } from "../config";
@@ -44,6 +48,7 @@ import type { Entry } from "../deployment-bundle/entry";
 import type { CfWorkerInit, CfPlacement } from "../deployment-bundle/worker";
 import type { PutConsumerBody } from "../queues/client";
 import type { AssetPaths } from "../sites";
+import type { RetrieveSourceMapFunction } from "../sourcemap";
 
 type Props = {
 	config: Config;
@@ -593,10 +598,11 @@ See https://developers.cloudflare.com/workers/platform/compatibility-dates for m
 		const placement: CfPlacement | undefined =
 			config.placement?.mode === "smart" ? { mode: "smart" } : undefined;
 
+		const entryPointName = path.basename(resolvedEntryPointPath);
 		const worker: CfWorkerInit = {
 			name: scriptName,
 			main: {
-				name: path.basename(resolvedEntryPointPath),
+				name: entryPointName,
 				filePath: resolvedEntryPointPath,
 				content: content,
 				type: bundleType,
@@ -689,6 +695,34 @@ See https://developers.cloudflare.com/workers/platform/compatibility-dates for m
 				}
 			} catch (err) {
 				helpIfErrorIsSizeOrScriptStartup(err, dependencies);
+
+				// Apply source mapping to validation startup errors if possible
+				if (
+					err instanceof ParseError &&
+					"code" in err &&
+					err.code === 10021 /* validation error */ &&
+					err.notes.length > 0
+				) {
+					const maybeNameToFilePath = (moduleName: string) => {
+						// If this is a service worker, always return the entrypoint path.
+						// Service workers can't have additional JavaScript modules.
+						if (bundleType === "commonjs") return resolvedEntryPointPath;
+						// Similarly, if the name matches the entrypoint, return its path
+						if (moduleName === entryPointName) return resolvedEntryPointPath;
+						// Otherwise, return the file path of the matching module (if any)
+						for (const module of modules) {
+							if (moduleName === module.name) return module.filePath;
+						}
+					};
+					const retrieveSourceMap: RetrieveSourceMapFunction = (moduleName) =>
+						maybeRetrieveFileSourceMap(maybeNameToFilePath(moduleName));
+
+					err.notes[0].text = getSourceMappedString(
+						err.notes[0].text,
+						retrieveSourceMap
+					);
+				}
+
 				throw err;
 			}
 		}

packages/wrangler/src/sourcemap.ts

@@ -1,10 +1,69 @@
 import assert from "node:assert";
+import fs from "node:fs";
+import url from "node:url";
+import type { Options } from "@cspotcode/source-map-support";
 import type Protocol from "devtools-protocol";
 
+function maybeGetFile(filePath: string | URL) {
+	try {
+		return fs.readFileSync(filePath, "utf8");
+	} catch (e: unknown) {
+		const notFound =
+			typeof e === "object" && e !== null && "code" in e && e.code === "ENOENT";
+		if (!notFound) throw e;
+	}
+}
+
+export type RetrieveSourceMapFunction = NonNullable<
+	Options["retrieveSourceMap"]
+>;
+export function maybeRetrieveFileSourceMap(
+	filePath?: string
+): ReturnType<RetrieveSourceMapFunction> {
+	if (filePath === undefined) return null;
+	const contents = maybeGetFile(filePath);
+	if (contents === undefined) return null;
+
+	// Find the last source mapping URL if any
+	const mapRegexp = /# sourceMappingURL=(.+)/g;
+	const matches = [...contents.matchAll(mapRegexp)];
+	// If we couldn't find a source mapping URL, there's nothing we can do
+	if (matches.length === 0) return null;
+	const mapMatch = matches[matches.length - 1];
+
+	// Get the source map
+	const fileUrl = url.pathToFileURL(filePath);
+	const mapUrl = new URL(mapMatch[1], fileUrl);
+	if (
+		mapUrl.protocol === "data:" &&
+		mapUrl.pathname.startsWith("application/json;base64,")
+	) {
+		// sourceMappingURL=data:application/json;base64,ew...
+		const base64 = mapUrl.href.substring(mapUrl.href.indexOf(",") + 1);
+		const map = Buffer.from(base64, "base64").toString();
+		return { map, url: fileUrl.href };
+	} else {
+		const map = maybeGetFile(mapUrl);
+		if (map === undefined) return null;
+		return { map, url: mapUrl.href };
+	}
+}
+
+// `sourceMappingPrepareStackTrace` is initialised on the first call to
+// `getSourceMappingPrepareStackTrace()`. Subsequent calls to
+// `getSourceMappingPrepareStackTrace()` will not update it. We'd like to be
+// able to customise source map retrieval on each call though. Therefore, we
+// make `retrieveSourceMapOverride` a module level variable, so
+// `sourceMappingPrepareStackTrace` always has access to the latest override.
 let sourceMappingPrepareStackTrace: typeof Error.prepareStackTrace;
-function getSourceMappingPrepareStackTrace(): NonNullable<
-	typeof Error.prepareStackTrace
-> {
+let retrieveSourceMapOverride: RetrieveSourceMapFunction | undefined;
+
+function getSourceMappingPrepareStackTrace(
+	retrieveSourceMap?: RetrieveSourceMapFunction
+): NonNullable<typeof Error.prepareStackTrace> {
+	// Source mapping is synchronous, so setting a module level variable is fine
+	retrieveSourceMapOverride = retrieveSourceMap;
+	// If we already have a source mapper, return it
 	if (sourceMappingPrepareStackTrace !== undefined) {
 		return sourceMappingPrepareStackTrace;
 	}
@@ -31,6 +90,10 @@ function getSourceMappingPrepareStackTrace(): NonNullable<
 		redirectConflictingLibrary: false,
 		// Make sure we're using fresh copies of files each time we source map
 		emptyCacheBetweenOperations: true,
+		// Allow retriever to be overridden at prepare stack trace time
+		retrieveSourceMap(path) {
+			return retrieveSourceMapOverride?.(path) ?? null;
+		},
 	});
 	sourceMappingPrepareStackTrace = Error.prepareStackTrace;
 	assert(sourceMappingPrepareStackTrace !== undefined);
@@ -69,7 +132,10 @@ function callFrameToCallSite(frame: Protocol.Runtime.CallFrame): CallSite {
 }
 
 const placeholderError = new Error();
-export function getSourceMappedString(value: string): string {
+export function getSourceMappedString(
+	value: string,
+	retrieveSourceMap?: RetrieveSourceMapFunction
+): string {
 	// We could use `.replace()` here with a function replacer, but
 	// `getSourceMappingPrepareStackTrace()` clears its source map caches between
 	// operations. It's likely call sites in this `value` will share source maps,
@@ -80,7 +146,8 @@ export function getSourceMappedString(value: string): string {
 	// of the call site would've been replaced with the same thing.
 	const callSiteLines = Array.from(value.matchAll(CALL_SITE_REGEXP));
 	const callSites = callSiteLines.map(lineMatchToCallSite);
-	const sourceMappedStackTrace: string = getSourceMappingPrepareStackTrace()(
+	const prepareStack = getSourceMappingPrepareStackTrace(retrieveSourceMap);
+	const sourceMappedStackTrace: string = prepareStack(
 		placeholderError,
 		callSites
 	);