From c37d94b51f4d5517c244f8a4178be6a266d2362e Mon Sep 17 00:00:00 2001 From: MrBBot Date: Wed, 10 Jan 2024 10:54:38 +0000 Subject: [PATCH] fix: ensure `miniflare`/`wrangler` can source map in the same process (#4719) --- .changeset/odd-fans-own.md | 8 ++++ fixtures/worker-app/src/index.js | 1 + fixtures/worker-app/tests/index.test.ts | 10 ++++ packages/miniflare/package.json | 1 - .../src/plugins/core/errors/index.ts | 2 + .../src/plugins/core/errors/sourcemap.ts | 47 ++++++++++++++----- packages/miniflare/src/plugins/index.ts | 1 + packages/wrangler/package.json | 2 +- packages/wrangler/scripts/deps.ts | 2 - packages/wrangler/src/sourcemap.ts | 16 ++----- pnpm-lock.yaml | 9 ++-- 11 files changed, 65 insertions(+), 34 deletions(-) create mode 100644 .changeset/odd-fans-own.md diff --git a/.changeset/odd-fans-own.md b/.changeset/odd-fans-own.md new file mode 100644 index 000000000000..603eb4f498a0 --- /dev/null +++ b/.changeset/odd-fans-own.md @@ -0,0 +1,8 @@ +--- +"miniflare": patch +"wrangler": patch +--- + +fix: ensure `miniflare` and `wrangler` can source map in the same process + +Previously, if in a `wrangler dev` session you called `console.log()` and threw an unhandled error you'd see an error like `[ERR_ASSERTION]: The expression evaluated to a falsy value`. This change ensures you can do both of these things in the same session. diff --git a/fixtures/worker-app/src/index.js b/fixtures/worker-app/src/index.js index 6f74ecac6bcb..9f4d4a0ed742 100644 --- a/fixtures/worker-app/src/index.js +++ b/fixtures/worker-app/src/index.js @@ -17,6 +17,7 @@ export default { const { pathname } = new URL(request.url); if (pathname === "/random") return new Response(hexEncode(randomBytes(8))); + if (pathname === "/error") throw new Error("Oops!"); console.log( request.method, diff --git a/fixtures/worker-app/tests/index.test.ts b/fixtures/worker-app/tests/index.test.ts index 8f1bed875afb..a6409ebc5a0d 100644 --- a/fixtures/worker-app/tests/index.test.ts +++ b/fixtures/worker-app/tests/index.test.ts @@ -45,6 +45,16 @@ describe("'wrangler dev' correctly renders pages", () => { ); }); + it("renders pretty error after logging", async ({ expect }) => { + // Regression test for https://github.com/cloudflare/workers-sdk/issues/4715 + const response = await fetch(`http://${ip}:${port}/error`); + const text = await response.text(); + expect(text).toContain("Oops!"); + expect(response.headers.get("Content-Type")).toBe( + "text/html;charset=utf-8" + ); + }); + it("uses `workerd` condition when bundling", async ({ expect }) => { const response = await fetch(`http://${ip}:${port}/random`); const text = await response.text(); diff --git a/packages/miniflare/package.json b/packages/miniflare/package.json index c99dde8c9198..a21546d09258 100644 --- a/packages/miniflare/package.json +++ b/packages/miniflare/package.json @@ -65,7 +65,6 @@ "@types/http-cache-semantics": "^4.0.1", "@types/node": "^18.11.9", "@types/rimraf": "^3.0.2", - "@types/source-map-support": "^0.5.6", "@types/stoppable": "^1.1.1", "@types/which": "^2.0.1", "@types/ws": "^8.5.3", diff --git a/packages/miniflare/src/plugins/core/errors/index.ts b/packages/miniflare/src/plugins/core/errors/index.ts index 1ba5ebea7b7a..0549d45e3ff3 100644 --- a/packages/miniflare/src/plugins/core/errors/index.ts +++ b/packages/miniflare/src/plugins/core/errors/index.ts @@ -274,3 +274,5 @@ export async function handlePrettyErrorRequest( headers: { "Content-Type": "text/html;charset=utf-8" }, }); } + +export { getFreshSourceMapSupport } from "./sourcemap"; diff --git a/packages/miniflare/src/plugins/core/errors/sourcemap.ts b/packages/miniflare/src/plugins/core/errors/sourcemap.ts index 5bc29fc032dc..772ce0d4179e 100644 --- a/packages/miniflare/src/plugins/core/errors/sourcemap.ts +++ b/packages/miniflare/src/plugins/core/errors/sourcemap.ts @@ -2,6 +2,40 @@ import assert from "assert"; import type { Options } from "@cspotcode/source-map-support"; import { parseStack } from "./callsite"; +// `source-map-support` will only modify `Error.prepareStackTrace` if this is +// the first time `install()` has been called. This is governed by shared data +// stored using a well-known symbol on `globalThis`. To ensure... +// +// a) `miniflare` and `wrangler` can have differing `install()` options +// b) We're not affecting external user's use of this package +// c) `Error.prepareStackTrace` is always updated on `install()` +// +// ...load a fresh copy, by resetting then restoring the `require` cache, and +// overriding `Symbol.for()` to return a unique symbol. +export function getFreshSourceMapSupport(): typeof import("@cspotcode/source-map-support") { + const resolvedSupportPath = require.resolve("@cspotcode/source-map-support"); + + const originalSymbolFor = Symbol.for; + const originalSupport = require.cache[resolvedSupportPath]; + try { + Symbol.for = (key) => { + // Make sure we only override the expected symbol. If we upgrade this + // package, and new symbols are used, this assertion will fail in tests. + // We want to guard against `source-map-support/sharedData` changing to + // something else. If this new symbol *should* be shared across package + // instances, we'll need to add an + // `if (key === "...") return originalSymbolFor(key);` here. + assert.strictEqual(key, "source-map-support/sharedData"); + return Symbol(key); + }; + delete require.cache[resolvedSupportPath]; + return require(resolvedSupportPath); + } finally { + Symbol.for = originalSymbolFor; + require.cache[resolvedSupportPath] = originalSupport; + } +} + const sourceMapInstallBaseOptions: Options = { environment: "node", // Don't add Node `uncaughtException` handler @@ -30,18 +64,7 @@ let sourceMapper: SourceMapper; export function getSourceMapper(): SourceMapper { if (sourceMapper !== undefined) return sourceMapper; - // `source-map-support` will only modify `Error.prepareStackTrace` if this is - // the first time `install()` has been called. This is governed by a module - // level variable: `errorFormatterInstalled`. To ensure we're not affecting - // external user's use of this package, and so `Error.prepareStackTrace` is - // always updated, load a fresh copy, by resetting then restoring the - // `require` cache. - - const originalSupport = require.cache["@cspotcode/source-map-support"]; - delete require.cache["@cspotcode/source-map-support"]; - const support: typeof import("@cspotcode/source-map-support") = require("@cspotcode/source-map-support"); - require.cache["@cspotcode/source-map-support"] = originalSupport; - + const support = getFreshSourceMapSupport(); const originalPrepareStackTrace = Error.prepareStackTrace; support.install(sourceMapInstallBaseOptions); const prepareStackTrace = Error.prepareStackTrace; diff --git a/packages/miniflare/src/plugins/index.ts b/packages/miniflare/src/plugins/index.ts index e7dd465283d1..e4b156c0dd1b 100644 --- a/packages/miniflare/src/plugins/index.ts +++ b/packages/miniflare/src/plugins/index.ts @@ -93,6 +93,7 @@ export { ModuleDefinitionSchema, SourceOptionsSchema, ProxyClient, + getFreshSourceMapSupport, } from "./core"; export type { ModuleRuleType, diff --git a/packages/wrangler/package.json b/packages/wrangler/package.json index 1400c4aa1e5a..67d3a403a6e3 100644 --- a/packages/wrangler/package.json +++ b/packages/wrangler/package.json @@ -102,7 +102,6 @@ }, "dependencies": { "@cloudflare/kv-asset-handler": "^0.2.0", - "@cspotcode/source-map-support": "0.8.1", "@esbuild-plugins/node-globals-polyfill": "^0.2.3", "@esbuild-plugins/node-modules-polyfill": "^0.2.2", "blake3-wasm": "^2.1.5", @@ -124,6 +123,7 @@ "@cloudflare/types": "^6.18.4", "@cloudflare/workers-tsconfig": "workspace:*", "@cloudflare/workers-types": "^4.20230914.0", + "@cspotcode/source-map-support": "0.8.1", "@iarna/toml": "^3.0.0", "@microsoft/api-extractor": "^7.28.3", "@sentry/node": "^7.86.0", diff --git a/packages/wrangler/scripts/deps.ts b/packages/wrangler/scripts/deps.ts index d0b54c72ed7a..493a7c313070 100644 --- a/packages/wrangler/scripts/deps.ts +++ b/packages/wrangler/scripts/deps.ts @@ -12,8 +12,6 @@ export const EXTERNAL_DEPENDENCIES = [ // todo - bundle miniflare too "selfsigned", "source-map", - // CommonJS module using `module.require()` which isn't provided by `esbuild` - "@cspotcode/source-map-support", "@esbuild-plugins/node-globals-polyfill", "@esbuild-plugins/node-modules-polyfill", "chokidar", diff --git a/packages/wrangler/src/sourcemap.ts b/packages/wrangler/src/sourcemap.ts index 4ba17ce0384a..47f7a68987fa 100644 --- a/packages/wrangler/src/sourcemap.ts +++ b/packages/wrangler/src/sourcemap.ts @@ -1,6 +1,7 @@ import assert from "node:assert"; import fs from "node:fs"; import url from "node:url"; +import { getFreshSourceMapSupport } from "miniflare"; import type { Options } from "@cspotcode/source-map-support"; import type Protocol from "devtools-protocol"; @@ -68,18 +69,9 @@ function getSourceMappingPrepareStackTrace( return sourceMappingPrepareStackTrace; } - // `source-map-support` will only modify `Error.prepareStackTrace` if this - // is the first time `install()` has been called. This is governed by a - // module level variable: `errorFormatterInstalled`. To ensure we're not - // affecting external user's use of this package, and so - // `Error.prepareStackTrace` is always updated, load a fresh copy, by - // resetting then restoring the `require` cache. - const originalSupport = require.cache["@cspotcode/source-map-support"]; - delete require.cache["@cspotcode/source-map-support"]; - // eslint-disable-next-line @typescript-eslint/consistent-type-imports,@typescript-eslint/no-var-requires - const support: typeof import("@cspotcode/source-map-support") = require("@cspotcode/source-map-support"); - require.cache["@cspotcode/source-map-support"] = originalSupport; - + // eslint-disable-next-line @typescript-eslint/consistent-type-imports + const support: typeof import("@cspotcode/source-map-support") = + getFreshSourceMapSupport(); const originalPrepareStackTrace = Error.prepareStackTrace; support.install({ environment: "node", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 3debe9e3e8f8..4c16bd55d282 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -870,9 +870,6 @@ importers: '@types/rimraf': specifier: ^3.0.2 version: 3.0.2 - '@types/source-map-support': - specifier: ^0.5.6 - version: 0.5.7 '@types/stoppable': specifier: ^1.1.1 version: 1.1.3 @@ -1255,9 +1252,6 @@ importers: '@cloudflare/kv-asset-handler': specifier: ^0.2.0 version: 0.2.0 - '@cspotcode/source-map-support': - specifier: 0.8.1 - version: 0.8.1 '@esbuild-plugins/node-globals-polyfill': specifier: ^0.2.3 version: 0.2.3(esbuild@0.17.19) @@ -1320,6 +1314,9 @@ importers: '@cloudflare/workers-types': specifier: ^4.20230914.0 version: 4.20230914.0 + '@cspotcode/source-map-support': + specifier: 0.8.1 + version: 0.8.1 '@iarna/toml': specifier: ^3.0.0 version: 3.0.0