From b2d094e52b519decf8fdef1bb8dcd42d3e4ac2ad Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Wed, 2 Oct 2024 17:43:02 +0100 Subject: [PATCH] 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 | 15 +++ .../wrangler/src/__tests__/deploy.test.ts | 119 ++++++++++++++++++ .../esbuild-plugins/cloudflare-internal.ts | 24 +++- .../esbuild-plugins/hybrid-nodejs-compat.ts | 36 +++++- .../esbuild-plugins/nodejs-compat.ts | 53 ++++++-- 5 files changed, 233 insertions(+), 14 deletions(-) create mode 100644 .changeset/perfect-experts-draw.md diff --git a/.changeset/perfect-experts-draw.md b/.changeset/perfect-experts-draw.md new file mode 100644 index 000000000000..3cd85c4cb6c1 --- /dev/null +++ b/.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 diff --git a/packages/wrangler/src/__tests__/deploy.test.ts b/packages/wrangler/src/__tests__/deploy.test.ts index ccf3e2a8d5a5..9256f5dce28a 100644 --- a/packages/wrangler/src/__tests__/deploy.test.ts +++ b/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({ diff --git a/packages/wrangler/src/deployment-bundle/esbuild-plugins/cloudflare-internal.ts b/packages/wrangler/src/deployment-bundle/esbuild-plugins/cloudflare-internal.ts index bc4885cf25f3..82b5703725a0 100644 --- a/packages/wrangler/src/deployment-bundle/esbuild-plugins/cloudflare-internal.ts +++ b/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/. + ` + ); + } + }); }, }; diff --git a/packages/wrangler/src/deployment-bundle/esbuild-plugins/hybrid-nodejs-compat.ts b/packages/wrangler/src/deployment-bundle/esbuild-plugins/hybrid-nodejs-compat.ts index de443ac1ec37..51af155293ed 100644 --- a/packages/wrangler/src/deployment-bundle/esbuild-plugins/hybrid-nodejs-compat.ts +++ b/packages/wrangler/src/deployment-bundle/esbuild-plugins/hybrid-nodejs-compat.ts @@ -12,6 +12,7 @@ export const nodejsHybridPlugin: () => Plugin = () => { return { name: "unenv-cloudflare", setup(build) { + errorOnServiceWorkerFormat(build); handleRequireCallsToNodeJSBuiltins(build); handleAliasedNodeJSPackages(build, alias, external); handleNodeJSGlobals(build, inject); @@ -19,15 +20,44 @@ export const nodejsHybridPlugin: () => Plugin = () => { }; }; +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 { diff --git a/packages/wrangler/src/deployment-bundle/esbuild-plugins/nodejs-compat.ts b/packages/wrangler/src/deployment-bundle/esbuild-plugins/nodejs-compat.ts index b184d82f2eab..6d57de18332b 100644 --- a/packages/wrangler/src/deployment-bundle/esbuild-plugins/nodejs-compat.ts +++ b/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"); +}