fix: render a helpful build error if a Service Worker mode Worker has…

… imports (#6872)

* fix: render a helpful build error if a Service Worker mode Worker has imports

A common mistake is to forget to export from the entry-point of a Worker, which causes
Wrangler to infer that we are in "Service Worker" mode.

In this mode, imports to external modules are not allowed.
Currently this only fails at runtime, because our esbuild step converts these imports to an internal `__require()` call that throws an error.
The error message is misleading and does not help the user identify the cause of the problem.
This is particularly tricky where the external imports are added by a library or our own node.js polyfills.

Fixes #6648

* fixup! fix: render a helpful build error if a Service Worker mode Worker has imports

* fixup! fix: render a helpful build error if a Service Worker mode Worker has imports

.changeset/perfect-experts-draw.md

@@ -0,0 +1,15 @@
+---
+"wrangler": patch
+---
+
+fix: render a helpful build error if a Service Worker mode Worker has imports
+
+A common mistake is to forget to export from the entry-point of a Worker, which causes
+Wrangler to infer that we are in "Service Worker" mode.
+
+In this mode, imports to external modules are not allowed.
+Currently this only fails at runtime, because our esbuild step converts these imports to an internal `__require()` call that throws an error.
+The error message is misleading and does not help the user identify the cause of the problem.
+This is particularly tricky where the external imports are added by a library or our own node.js polyfills.
+
+Fixes #6648

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

@@ -8876,6 +8876,125 @@ addEventListener('fetch', event => {});`
 		});
 	});
 
+	describe("service worker format", () => {
+		it("should error if trying to import a cloudflare prefixed external when in service worker format", async () => {
+			writeWranglerToml();
+			fs.writeFileSync(
+				"dep-1.js",
+				dedent`
+					import sockets from 'cloudflare:sockets';
+					export const external = sockets;
+				`
+			);
+			fs.writeFileSync(
+				"dep-2.js",
+				dedent`
+					export const internal = 100;
+				`
+			);
+			fs.writeFileSync(
+				"index.js",
+				dedent`
+					import {external} from "./dep-1"; // will the external import check be transitive?
+					import {internal} from "./dep-2"; // ensure that we can still have a non-external import
+					let x = [external, internal]; // to ensure that esbuild doesn't tree shake the imports
+					// no default export making this a service worker format
+					addEventListener('fetch', (event) => {
+						event.respondWith(new Response(''));
+					});
+			`
+			);
+
+			await expect(
+				runWrangler("deploy index.js --dry-run").catch(
+					(e) =>
+						normalizeString(
+							esbuild
+								.formatMessagesSync(e?.errors ?? [], { kind: "error" })
+								.join()
+								.trim()
+						).split("This error came from the")[0]
+				)
+			).resolves.toMatchInlineSnapshot(`
+				"X [ERROR] Unexpected external import of \\"cloudflare:sockets\\". Imports are not valid in a Service Worker format Worker.
+				Did you mean to create a Module Worker?
+				If so, try adding \`export default { ... }\` in your entry-point.
+				See https://developers.cloudflare.com/workers/reference/migrate-to-module-workers/. [plugin Cloudflare internal imports plugin]
+
+				  "
+			`);
+		});
+
+		it("should error if importing a node.js library when in service worker format", async () => {
+			writeWranglerToml();
+			fs.writeFileSync(
+				"index.js",
+				dedent`
+					import stream from "node:stream";
+					let temp = stream;
+					addEventListener('fetch', (event) => {
+						event.respondWith(new Response(''));
+					});
+			`
+			);
+
+			await expect(
+				runWrangler("deploy index.js --dry-run").catch(
+					(e) =>
+						normalizeString(
+							esbuild
+								.formatMessagesSync(e?.errors ?? [], { kind: "error" })
+								.join()
+								.trim()
+						).split("This error came from the")[0]
+				)
+			).resolves.toMatchInlineSnapshot(`
+				"X [ERROR]
+										Unexpected external import of \\"node:stream\\". Imports are not valid in a Service Worker format Worker.
+										Did you mean to create a Module Worker?
+										If so, try adding \`export default { ... }\` in your entry-point.
+										See https://developers.cloudflare.com/workers/reference/migrate-to-module-workers/.
+									 [plugin nodejs_compat imports plugin]
+
+				  "
+			`);
+		});
+
+		it("should error if nodejs_compat (v2) is turned on when in service worker format", async () => {
+			writeWranglerToml({
+				compatibility_date: "2024-09-23", // Sept 23 to turn on nodejs compat v2 mode
+				compatibility_flags: ["nodejs_compat"],
+			});
+			fs.writeFileSync(
+				"index.js",
+				dedent`
+					addEventListener('fetch', (event) => {
+						event.respondWith(new Response(''));
+					});
+			`
+			);
+
+			await expect(
+				runWrangler("deploy index.js --dry-run").catch(
+					(e) =>
+						normalizeString(
+							esbuild
+								.formatMessagesSync(e?.errors ?? [], { kind: "error" })
+								.join()
+								.trim()
+						).split("This error came from the")[0]
+				)
+			).resolves.toMatchInlineSnapshot(`
+				"X [ERROR] Unexpected external import of \\"node:stream\\" and \\"node:timers/promises\\". Imports are not valid in a Service Worker format Worker.
+				Did you mean to create a Module Worker?
+				If so, try adding \`export default { ... }\` in your entry-point.
+				See https://developers.cloudflare.com/workers/reference/migrate-to-module-workers/. [plugin unenv-cloudflare]
+
+				  "
+			`);
+		});
+	});
+
 	describe("legacy module specifiers", () => {
 		it("should work with legacy module specifiers, with a deprecation warning (1)", async () => {
 			writeWranglerToml({

packages/wrangler/src/deployment-bundle/esbuild-plugins/cloudflare-internal.ts

@@ -1,3 +1,4 @@
+import { dedent } from "../../utils/dedent";
 import type { Plugin } from "esbuild";
 
 /**
@@ -6,8 +7,29 @@ import type { Plugin } from "esbuild";
 export const cloudflareInternalPlugin: Plugin = {
 	name: "Cloudflare internal imports plugin",
 	setup(pluginBuild) {
-		pluginBuild.onResolve({ filter: /^cloudflare:.*/ }, () => {
+		const paths = new Set();
+		pluginBuild.onStart(() => paths.clear());
+		pluginBuild.onResolve({ filter: /^cloudflare:.*/ }, (args) => {
+			paths.add(args.path);
 			return { external: true };
 		});
+		pluginBuild.onEnd(() => {
+			if (pluginBuild.initialOptions.format === "iife" && paths.size > 0) {
+				// If we are bundling in "Service Worker" mode,
+				// imports of external modules such as `cloudflare:...`,
+				// which won't be inlined/bundled by esbuild, are invalid.
+				const pathList = new Intl.ListFormat("en-US").format(
+					Array.from(paths.keys()).map((p) => `"${p}"`)
+				);
+				throw new Error(
+					dedent`
+						Unexpected external import of ${pathList}. Imports are not valid in a Service Worker format Worker.
+						Did you mean to create a Module Worker?
+						If so, try adding \`export default { ... }\` in your entry-point.
+						See https://developers.cloudflare.com/workers/reference/migrate-to-module-workers/.
+					`
+				);
+			}
+		});
 	},
 };

packages/wrangler/src/deployment-bundle/esbuild-plugins/hybrid-nodejs-compat.ts

@@ -12,22 +12,52 @@ export const nodejsHybridPlugin: () => Plugin = () => {
 	return {
 		name: "unenv-cloudflare",
 		setup(build) {
+			errorOnServiceWorkerFormat(build);
 			handleRequireCallsToNodeJSBuiltins(build);
 			handleAliasedNodeJSPackages(build, alias, external);
 			handleNodeJSGlobals(build, inject);
 		},
 	};
 };
 
+const NODEJS_MODULES_RE = new RegExp(`^(node:)?(${builtinModules.join("|")})$`);
+
+/**
+ * If we are bundling a "Service Worker" formatted Worker, imports of external modules,
+ * which won't be inlined/bundled by esbuild, are invalid.
+ *
+ * This `onResolve()` handler will error if it identifies node.js external imports.
+ */
+function errorOnServiceWorkerFormat(build: PluginBuild) {
+	const paths = new Set();
+	build.onStart(() => paths.clear());
+	build.onResolve({ filter: NODEJS_MODULES_RE }, (args) => {
+		paths.add(args.path);
+		return null;
+	});
+	build.onEnd(() => {
+		if (build.initialOptions.format === "iife" && paths.size > 0) {
+			const pathList = new Intl.ListFormat("en-US").format(
+				Array.from(paths.keys()).map((p) => `"${p}"`)
+			);
+			throw new Error(
+				dedent`
+					Unexpected external import of ${pathList}. Imports are not valid in a Service Worker format Worker.
+					Did you mean to create a Module Worker?
+					If so, try adding \`export default { ... }\` in your entry-point.
+					See https://developers.cloudflare.com/workers/reference/migrate-to-module-workers/.
+				`
+			);
+		}
+	});
+}
+
 /**
  * We must convert `require()` calls for Node.js to a virtual ES Module that can be imported avoiding the require calls.
  * We do this by creating a special virtual ES module that re-exports the library in an onLoad handler.
  * The onLoad handler is triggered by matching the "namespace" added to the resolve.
  */
 function handleRequireCallsToNodeJSBuiltins(build: PluginBuild) {
-	const NODEJS_MODULES_RE = new RegExp(
-		`^(node:)?(${builtinModules.join("|")})$`
-	);
 	build.onResolve({ filter: NODEJS_MODULES_RE }, (args) => {
 		if (args.kind === "require-call") {
 			return {

packages/wrangler/src/deployment-bundle/esbuild-plugins/nodejs-compat.ts

@@ -1,6 +1,7 @@
 import { relative } from "path";
 import chalk from "chalk";
 import { logger } from "../../logger";
+import { dedent } from "../../utils/dedent";
 import type { Plugin } from "esbuild";
 
 /**
@@ -53,25 +54,57 @@ export const nodejsCompatPlugin: (silenceWarnings: boolean) => Plugin = (
 				return result;
 			}
 		);
+
+		/**
+		 * If we are bundling a "Service Worker" formatted Worker, imports of external modules,
+		 * which won't be inlined/bundled by esbuild, are invalid.
+		 *
+		 * This `onEnd()` handler will error if it identifies node.js external imports.
+		 */
+		pluginBuild.onEnd(() => {
+			if (
+				pluginBuild.initialOptions.format === "iife" &&
+				warnedPackaged.size > 0
+			) {
+				const paths = new Intl.ListFormat("en-US").format(
+					Array.from(warnedPackaged.keys()).map((p) => `"${p}"`)
+				);
+				throw new Error(`
+						Unexpected external import of ${paths}. Imports are not valid in a Service Worker format Worker.
+						Did you mean to create a Module Worker?
+						If so, try adding \`export default { ... }\` in your entry-point.
+						See https://developers.cloudflare.com/workers/reference/migrate-to-module-workers/.
+					`);
+				const errors = Array.from(warnedPackaged.entries()).map(
+					([path, importers]) =>
+						`Unexpected import "${path}" which is not valid in a Service Worker format Worker. Are you missing \`export default { ... }\` from your Worker?\n` +
+						"Imported from:\n" +
+						toList(importers, pluginBuild.initialOptions.absWorkingDir) +
+						"\n"
+				);
+				throw new Error(errors.join(""));
+			}
+		});
+
 		// Wait until the build finishes to log warnings, so that all files which import a package
 		// can be collated
 		pluginBuild.onEnd(() => {
 			if (!silenceWarnings) {
 				warnedPackaged.forEach((importers: string[], path: string) => {
 					logger.warn(
-						`The package "${path}" wasn't found on the file system but is built into node.
-Your Worker may throw errors at runtime unless you enable the "nodejs_compat" compatibility flag. Refer to https://developers.cloudflare.com/workers/runtime-apis/nodejs/ for more details. Imported from:
-${importers
-	.map(
-		(i) =>
-			` - ${chalk.blue(
-				relative(pluginBuild.initialOptions.absWorkingDir ?? "/", i)
-			)}`
-	)
-	.join("\n")}`
+						dedent`
+						The package "${path}" wasn't found on the file system but is built into node.
+						Your Worker may throw errors at runtime unless you enable the "nodejs_compat" compatibility flag. Refer to https://developers.cloudflare.com/workers/runtime-apis/nodejs/ for more details. Imported from:
+						${toList(importers, pluginBuild.initialOptions.absWorkingDir)}`
 					);
 				});
 			}
 		});
 	},
 });
+
+function toList(items: string[], absWorkingDir: string | undefined): string {
+	return items
+		.map((i) => ` - ${chalk.blue(relative(absWorkingDir ?? "/", i))}`)
+		.join("\n");
+}