Skip to content

Commit

Permalink
fix: ensure worker reloads when importing node:* modules (#4962)
Browse files Browse the repository at this point in the history
* fix: ensure worker reloads when importing `node:*` modules

* fix: ensure conflicting compatibility flags reported as user error

* fix: add type arguments to `seen` and `warnedPackaged`
  • Loading branch information
mrbbot authored Feb 9, 2024
1 parent b3110c3 commit d658517
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 25 deletions.
7 changes: 7 additions & 0 deletions .changeset/cuddly-apples-divide.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"wrangler": patch
---

fix: ensure `wrangler dev` can reload without crashing when importing `node:*` modules

The previous Wrangler release introduced a regression that caused reloads to fail when importing `node:*` modules. This change fixes that, and ensures these modules can always be resolved.
7 changes: 6 additions & 1 deletion packages/wrangler/e2e/dev.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ describe("basic dev tests", () => {
name = "${workerName}"
main = "src/index.ts"
compatibility_date = "2023-01-01"
compatibility_flags = ["nodejs_compat"]
[vars]
KEY = "value"
Expand Down Expand Up @@ -195,9 +196,12 @@ describe("basic dev tests", () => {

await worker.seed({
"src/index.ts": dedent`
import { Buffer } from "node:buffer";
export default {
fetch(request, env) {
return new Response("Updated Worker! " + env.KEY)
const base64Message = Buffer.from("Updated Worker!").toString("base64");
const message = Buffer.from(base64Message, "base64").toString();
return new Response(message + " " + env.KEY)
}
}`,
});
Expand All @@ -216,6 +220,7 @@ describe("basic dev tests", () => {
name = "${workerName}"
main = "src/index.ts"
compatibility_date = "2023-01-01"
compatibility_flags = ["nodejs_compat"]
[vars]
KEY = "updated"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ describe("isNavigatorDefined", () => {
assert(false, "Unreachable");
} catch (e) {
expect(e).toMatchInlineSnapshot(
`[AssertionError: Can't both enable and disable a flag]`
`[Error: Can't both enable and disable a flag]`
);
}
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,6 @@ import chalk from "chalk";
import { logger } from "../../logger";
import type { Plugin } from "esbuild";

// Infinite loop detection
const seen = new Set();

// Prevent multiple warnings per package
const warnedPackaged = new Map();

/**
* An esbuild plugin that will mark any `node:...` imports as external.
*/
Expand All @@ -17,8 +11,16 @@ export const nodejsCompatPlugin: (silenceWarnings: boolean) => Plugin = (
) => ({
name: "nodejs_compat imports plugin",
setup(pluginBuild) {
seen.clear();
warnedPackaged.clear();
// Infinite loop detection
const seen = new Set<string>();

// Prevent multiple warnings per package
const warnedPackaged = new Map<string, string[]>();

pluginBuild.onStart(() => {
seen.clear();
warnedPackaged.clear();
});
pluginBuild.onResolve(
{ filter: /node:.*/ },
async ({ path, kind, resolveDir, ...opts }) => {
Expand All @@ -39,14 +41,12 @@ export const nodejsCompatPlugin: (silenceWarnings: boolean) => Plugin = (
// esbuild couldn't resolve the package
// We should warn the user, but not fail the build

if (!warnedPackaged.has(path)) {
warnedPackaged.set(path, [opts.importer]);
} else {
warnedPackaged.set(path, [
...warnedPackaged.get(path),
opts.importer,
]);
let pathWarnedPackaged = warnedPackaged.get(path);
if (pathWarnedPackaged === undefined) {
warnedPackaged.set(path, (pathWarnedPackaged = []));
}
pathWarnedPackaged.push(opts.importer);

return { external: true };
}
// This is a normal package—don't treat it specially
Expand Down
16 changes: 8 additions & 8 deletions packages/wrangler/src/navigator-user-agent.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
import assert from "node:assert";
import { UserError } from "./errors";

export function isNavigatorDefined(
compatibility_date: string | undefined,
compatibility_flags: string[] = []
) {
assert(
!(
compatibility_flags.includes("global_navigator") &&
compatibility_flags.includes("no_global_navigator")
),
"Can't both enable and disable a flag"
);
if (
compatibility_flags.includes("global_navigator") &&
compatibility_flags.includes("no_global_navigator")
) {
throw new UserError("Can't both enable and disable a flag");
}

if (compatibility_flags.includes("global_navigator")) {
return true;
}
Expand Down

0 comments on commit d658517

Please sign in to comment.