fix: tidy up error messaging for unexpected use of Node.js APIs

Fixes #6822

.changeset/early-tigers-buy.md

@@ -0,0 +1,7 @@
+---
+"wrangler": patch
+---
+
+fix: tidy up error messaging for unexpected use of Node.js APIs
+
+Fixes #6822

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

@@ -9256,27 +9256,103 @@ export default{
 			`);
 		});
 
-		it("should recommend node compatibility mode when using node builtins and node-compat isn't enabled", async () => {
+		it("should recommend node compatibility flag mode when using node builtins and no node compat is enabled", async () => {
 			writeWranglerToml();
-			fs.writeFileSync(
-				"index.js",
-				`
-      import path from 'path';
-      console.log(path.join("some/path/to", "a/file.txt"));
-      export default {}
-      `
-			);
-			let err: esbuild.BuildFailure | undefined;
-			try {
-				await runWrangler("deploy index.js --dry-run"); // expecting this to throw, as node compatibility isn't enabled
-			} catch (e) {
-				err = e as esbuild.BuildFailure;
-			}
-			expect(
-				esbuild.formatMessagesSync(err?.errors ?? [], { kind: "error" }).join()
-			).toMatch(
-				/The package "path" wasn't found on the file system but is built into node\.\s+Add "node_compat = true" to your wrangler\.toml file and make sure to prefix the module name with "node:" to enable Node.js compatibility\./
-			);
+			fs.writeFileSync("index.js", "import path from 'path';");
+
+			await expect(
+				runWrangler("deploy index.js --dry-run").catch((e) =>
+					esbuild
+						.formatMessagesSync(e?.errors ?? [], { kind: "error" })
+						.join()
+						.trim()
+				)
+			).resolves.toMatchInlineSnapshot(`
+				"✘ [ERROR] Could not resolve \\"path\\"
+
+				    index.js:1:17:
+				      1 │ import path from 'path';
+				        ╵                  ~~~~~~
+
+				  The package \\"path\\" wasn't found on the file system but is built into node.
+				  - Add the \\"nodejs_compat\\" compatibility flag to your project."
+			`);
+		});
+
+		it("should recommend node compatibility flag mode when using node builtins and node compat is set only to nodejs_als", async () => {
+			writeWranglerToml({
+				compatibility_flags: ["nodejs_als"],
+			});
+			fs.writeFileSync("index.js", "import path from 'path';");
+
+			await expect(
+				runWrangler("deploy index.js --dry-run").catch((e) =>
+					esbuild
+						.formatMessagesSync(e?.errors ?? [], { kind: "error" })
+						.join()
+						.trim()
+				)
+			).resolves.toMatchInlineSnapshot(`
+				"✘ [ERROR] Could not resolve \\"path\\"
+
+				    index.js:1:17:
+				      1 │ import path from 'path';
+				        ╵                  ~~~~~~
+
+				  The package \\"path\\" wasn't found on the file system but is built into node.
+				  - Add the \\"nodejs_compat\\" compatibility flag to your project."
+			`);
+		});
+
+		it("should recommend node compatibility flag mode when using node builtins and `node_compat` is true", async () => {
+			writeWranglerToml({
+				node_compat: true,
+			});
+			fs.writeFileSync("index.js", "import fs from 'diagnostics_channel';");
+
+			await expect(
+				runWrangler("deploy index.js --dry-run").catch((e) =>
+					esbuild
+						.formatMessagesSync(e?.errors ?? [], { kind: "error" })
+						.join()
+						.trim()
+				)
+			).resolves.toMatchInlineSnapshot(`
+				"✘ [ERROR] Could not resolve \\"diagnostics_channel\\"
+
+				    index.js:1:15:
+				      1 │ import fs from 'diagnostics_channel';
+				        ╵                ~~~~~~~~~~~~~~~~~~~~~
+
+				  The package \\"diagnostics_channel\\" wasn't found on the file system but is built into node.
+				  - Try removing the legacy \\"node_compat\\" setting and add the \\"nodejs_compat\\" compatibility flag in your project"
+			`);
+		});
+
+		it("should recommend updating the compatibility date mode when using node builtins and the `nodejs_compat` flag", async () => {
+			writeWranglerToml({
+				compatibility_date: "2024/09/01", // older than Sept 23rd, 2024
+				compatibility_flags: ["nodejs_compat"],
+			});
+			fs.writeFileSync("index.js", "import fs from 'path';");
+
+			await expect(
+				runWrangler("deploy index.js --dry-run").catch((e) =>
+					esbuild
+						.formatMessagesSync(e?.errors ?? [], { kind: "error" })
+						.join()
+						.trim()
+				)
+			).resolves.toMatchInlineSnapshot(`
+				"✘ [ERROR] Could not resolve \\"path\\"
+
+				    index.js:1:15:
+				      1 │ import fs from 'path';
+				        ╵                ~~~~~~
+
+				  The package \\"path\\" wasn't found on the file system but is built into node.
+				  - Make sure to prefix the module name with \\"node:\\" or update your compatibility_date to 2024/09/23 or later."
+			`);
 		});
 
 		it("should polyfill node builtins when enabled", async () => {

packages/wrangler/src/deployment-bundle/build-failures.ts

@@ -1,4 +1,5 @@
 import { builtinModules } from "node:module";
+import type { NodeJSCompatMode } from "./node-compat";
 import type * as esbuild from "esbuild";
 
 /**
@@ -19,23 +20,26 @@ const nodeBuiltinResolveErrorText = new RegExp(
  */
 export function rewriteNodeCompatBuildFailure(
 	errors: esbuild.Message[],
-	forPages = false
+	compatMode: NodeJSCompatMode = null
 ) {
 	for (const error of errors) {
 		const match = nodeBuiltinResolveErrorText.exec(error.text);
 		if (match !== null) {
-			const issue = `The package "${match[1]}" wasn't found on the file system but is built into node.`;
+			let text = `The package "${match[1]}" wasn't found on the file system but is built into node.\n`;
 
-			const instructionForUser = `${
-				forPages
-					? 'Add the "nodejs_compat" compatibility flag to your Pages project'
-					: 'Add "node_compat = true" to your wrangler.toml file'
-			} and make sure to prefix the module name with "node:" to enable Node.js compatibility.`;
+			if (compatMode === null || compatMode === "als") {
+				text += `- Add the "nodejs_compat" compatibility flag to your project.\n`;
+			} else if (compatMode === "legacy") {
+				text += `- Try removing the legacy "node_compat" setting and add the "nodejs_compat" compatibility flag in your project\n`;
+			}
+			if (compatMode === "v1" && !match[1].startsWith("node:")) {
+				text += `- Make sure to prefix the module name with "node:" or update your compatibility_date to 2024/09/23 or later.\n`;
+			}
 
 			error.notes = [
 				{
 					location: null,
-					text: `${issue}\n${instructionForUser}`,
+					text,
 				},
 			];
 		}

packages/wrangler/src/deployment-bundle/bundle.ts

@@ -133,7 +133,6 @@ export type BundleOptions = {
 	sourcemap?: esbuild.CommonOptions["sourcemap"];
 	plugins?: esbuild.Plugin[];
 	isOutfile?: boolean;
-	forPages?: boolean;
 	local: boolean;
 	projectRoot: string | undefined;
 	defineNavigatorUserAgent: boolean;
@@ -172,7 +171,6 @@ export async function bundleWorker(
 		sourcemap,
 		plugins,
 		isOutfile,
-		forPages,
 		local,
 		projectRoot,
 		defineNavigatorUserAgent,
@@ -485,8 +483,8 @@ export async function bundleWorker(
 			};
 		}
 	} catch (e) {
-		if (nodejsCompatMode !== "legacy" && isBuildFailure(e)) {
-			rewriteNodeCompatBuildFailure(e.errors, forPages);
+		if (isBuildFailure(e)) {
+			rewriteNodeCompatBuildFailure(e.errors, nodejsCompatMode);
 		}
 		throw e;
 	}

packages/wrangler/src/pages/functions/buildPlugin.ts

@@ -109,7 +109,6 @@ export function buildPluginFromFunctions({
 		// TODO: mock AE datasets in Pages functions for dev
 		mockAnalyticsEngineDatasets: [],
 		targetConsumer: local ? "dev" : "deploy",
-		forPages: true,
 		local,
 		projectRoot: getPagesProjectRoot(),
 		defineNavigatorUserAgent,

packages/wrangler/src/pages/functions/buildWorker.ts

@@ -85,7 +85,6 @@ export function buildWorkerFromFunctions({
 		serveLegacyAssetsFromWorker: false,
 		checkFetch: local,
 		targetConsumer: local ? "dev" : "deploy",
-		forPages: true,
 		local,
 		projectRoot: getPagesProjectRoot(),
 		defineNavigatorUserAgent,
@@ -189,7 +188,6 @@ export function buildRawWorker({
 		serveLegacyAssetsFromWorker: false,
 		checkFetch: local,
 		targetConsumer: local ? "dev" : "deploy",
-		forPages: true,
 		local,
 		projectRoot: getPagesProjectRoot(),
 		defineNavigatorUserAgent,