From 4ac1ed1f420f132c1ae79d7839c26cfc32527330 Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Wed, 25 Sep 2024 10:21:17 +0100 Subject: [PATCH] fix: tidy up error messaging for unexpected use of Node.js APIs Fixes #6822 --- .changeset/early-tigers-buy.md | 7 ++ .../wrangler/src/__tests__/deploy.test.ts | 116 +++++++++++++++--- .../src/deployment-bundle/build-failures.ts | 20 +-- .../wrangler/src/deployment-bundle/bundle.ts | 6 +- .../src/pages/functions/buildPlugin.ts | 1 - .../src/pages/functions/buildWorker.ts | 2 - 6 files changed, 117 insertions(+), 35 deletions(-) create mode 100644 .changeset/early-tigers-buy.md diff --git a/.changeset/early-tigers-buy.md b/.changeset/early-tigers-buy.md new file mode 100644 index 000000000000..fd1edfad238a --- /dev/null +++ b/.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 diff --git a/packages/wrangler/src/__tests__/deploy.test.ts b/packages/wrangler/src/__tests__/deploy.test.ts index 5111188eed90..cd776c1db070 100644 --- a/packages/wrangler/src/__tests__/deploy.test.ts +++ b/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 () => { diff --git a/packages/wrangler/src/deployment-bundle/build-failures.ts b/packages/wrangler/src/deployment-bundle/build-failures.ts index 11a2bd23c2c0..150bad15f66b 100644 --- a/packages/wrangler/src/deployment-bundle/build-failures.ts +++ b/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, }, ]; } diff --git a/packages/wrangler/src/deployment-bundle/bundle.ts b/packages/wrangler/src/deployment-bundle/bundle.ts index 24765fc39379..1ec89d15f24f 100644 --- a/packages/wrangler/src/deployment-bundle/bundle.ts +++ b/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; } diff --git a/packages/wrangler/src/pages/functions/buildPlugin.ts b/packages/wrangler/src/pages/functions/buildPlugin.ts index 4f0d8346e075..867a8dfa3fa9 100644 --- a/packages/wrangler/src/pages/functions/buildPlugin.ts +++ b/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, diff --git a/packages/wrangler/src/pages/functions/buildWorker.ts b/packages/wrangler/src/pages/functions/buildWorker.ts index 9792b4314b39..b4c9223bd595 100644 --- a/packages/wrangler/src/pages/functions/buildWorker.ts +++ b/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,