fix: refactor Node.js compat support to ensure all polyfills are pre-bundled before the first request (#8176)

* fix: refactor Node.js compat support to ensure all polyfills are pre-bundled before the first request

* fixups after PR review

* add assertion

* rename globInject variable

Author: petebacondarwin

.changeset/ninety-phones-tell.md

@@ -0,0 +1,5 @@
+---
+"@cloudflare/vite-plugin": patch
+---
+
+fix: refactor Node.js compat support to ensure all polyfills are pre-bundled before the first request

packages/vite-plugin-cloudflare/src/cloudflare-environment.ts

@@ -1,7 +1,7 @@
 import assert from "node:assert";
 import * as vite from "vite";
 import { INIT_PATH, UNKNOWN_HOST } from "./shared";
-import { getOutputDirectory, nodeBuiltInModules } from "./utils";
+import { getOutputDirectory } from "./utils";
 import type { ResolvedPluginConfig, WorkerConfig } from "./plugin-config";
 import type { Fetcher } from "@cloudflare/workers-types/experimental";
 import type {
@@ -134,8 +134,6 @@ export function createCloudflareEnvironmentOptions(
 			conditions: [...defaultConditions, "development|production"],
 			// The Cloudflare ones are proper builtins in the environment
 			builtins: [...cloudflareBuiltInModules],
-			// The Node.js ones are no proper builtins in the environment since we also polyfill them using unenv
-			external: [...nodeBuiltInModules],
 		},
 		dev: {
 			createEnvironment(name, config) {
@@ -164,10 +162,6 @@ export function createCloudflareEnvironmentOptions(
 			// Note: ssr pre-bundling is opt-in and we need to enable it by setting `noDiscovery` to false
 			noDiscovery: false,
 			entries: workerConfig.main,
-			exclude: [
-				// we have to exclude all node modules to work in dev-mode not just the unenv externals...
-				...nodeBuiltInModules,
-			],
 			esbuildOptions: {
 				platform: "neutral",
 				conditions: [...defaultConditions, "development"],

packages/vite-plugin-cloudflare/src/index.ts

@@ -1,5 +1,6 @@
 import assert from "node:assert";
 import * as fs from "node:fs";
+import { builtinModules } from "node:module";
 import * as path from "node:path";
 import { createMiddleware } from "@hattip/adapter-node";
 import MagicString from "magic-string";
@@ -16,11 +17,10 @@ import {
 	getPreviewMiniflareOptions,
 } from "./miniflare-options";
 import {
-	getNodeCompatAliases,
+	getNodeCompatEntries,
 	getNodeCompatExternals,
 	injectGlobalCode,
 	isNodeCompat,
-	maybeStripNodeJsVirtualPrefix,
 	resolveNodeJSImport,
 } from "./node-js-compat";
 import { resolvePluginConfig } from "./plugin-config";
@@ -407,67 +407,68 @@ export function cloudflare(pluginConfig: PluginConfig = {}): vite.Plugin[] {
 				// Skip this whole plugin if we are in preview mode
 				return !env.isPreview;
 			},
-			config() {
-				// Configure Vite with the Node.js polyfill aliases
-				// We have to do this across the whole Vite config because it is not possible to do it per Environment.
-				// These aliases are to virtual modules that then get Environment specific handling in the resolveId hook.
-				return {
-					resolve: {
-						alias: getNodeCompatAliases(),
-					},
-				};
-			},
-			configEnvironment(environmentName) {
-				// Ignore Node.js external modules when building environments that use Node.js compat.
-				const workerConfig = getWorkerConfig(environmentName);
-				if (isNodeCompat(workerConfig)) {
+			configEnvironment(name) {
+				// Only configure this environment if it is a Worker using Node.js compatibility.
+				if (isNodeCompat(getWorkerConfig(name))) {
 					return {
-						build: {
-							rollupOptions: {
-								external: getNodeCompatExternals(),
-							},
+						resolve: {
+							builtins: getNodeCompatExternals(),
+						},
+						optimizeDeps: {
+							// This is a list of dependency entry-points that should be pre-bundled.
+							// In this case we provide a list of all the possible polyfills so that they are pre-bundled,
+							// ready ahead the first request to the dev server.
+							// Without this the dependency optimizer will try to bundle them on-the-fly in the middle of the first request,
+							// which can potentially cause problems if it leads to previous pre-bundling to become stale and needing to be reloaded.
+							include: [...getNodeCompatEntries()],
+							// This is a list of module specifiers that the dependency optimizer should not follow when doing import analysis.
+							// In this case we provide a list of all the Node.js modules, both those built-in to workerd and those that will be polyfilled.
+							// Obviously we don't want/need the optimizer to try to process modules that are built-in;
+							// But also we want to avoid following the ones that are polyfilled since the dependency-optimizer import analyzer does not
+							// resolve these imports using our `resolveId()` hook causing the optimization step to fail.
+							exclude: [
+								...builtinModules,
+								...builtinModules.map((m) => `node:${m}`),
+							],
 						},
 					};
 				}
 			},
+			applyToEnvironment(environment) {
+				// Only run this plugin's runtime hooks if it is a Worker using Node.js compatibility.
+				return isNodeCompat(getWorkerConfig(environment.name));
+			},
+			// We need the resolver from this plugin to run before built-in ones, otherwise Vite's built-in
+			// resolver will try to externalize the Node.js module imports (e.g. `perf_hooks` and `node:tty`)
+			// rather than allowing the resolve hook here to alias then to polyfills.
+			enforce: "pre",
 			async resolveId(source, importer, options) {
-				// Handle the virtual modules that come from Node.js compat aliases.
-				const strippedSource = maybeStripNodeJsVirtualPrefix(source);
-				if (!strippedSource) {
-					return;
+				// See if we can map the `source` to a Node.js compat alias.
+				const result = resolveNodeJSImport(source);
+				if (!result) {
+					// The source is not a Node.js compat alias so just pass it through
+					return this.resolve(source, importer, options);
 				}
 
-				const workerConfig = getWorkerConfig(this.environment.name);
-				if (!isNodeCompat(workerConfig)) {
-					// We are not in Node.js compat mode, so we must not try to apply any unenv aliases.
-					// Just resolve the module id as normal.
-					return this.resolve(strippedSource, importer, options);
-				}
-
-				// Resolve this id following any unenv aliases.
-				const { unresolved, resolved } = resolveNodeJSImport(strippedSource);
-
-				if (this.environment.mode === "dev" && this.environment.depsOptimizer) {
-					// Make sure the dependency optimizer is aware of this aliased import
-					const optimized =
-						this.environment.depsOptimizer.registerMissingImport(
-							unresolved,
-							resolved
-						).id;
-					// We must pass the id from the depsOptimizer through the rest of the
-					// resolving pipeline to ensure that the optimized version gets used.
-					return this.resolve(optimized, importer, options);
+				if (this.environment.mode === "dev") {
+					assert(
+						this.environment.depsOptimizer,
+						"depsOptimizer is required in dev mode"
+					);
+					// We are in dev mode (rather than build).
+					// Node.js compat polyfill modules have already been pre-bundled,
+					// so we can use the unresolved path to the polyfill
+					// and let the dependency optimizer resolve the path to the pre-bundled version.
+					return this.resolve(result.unresolved, importer, options);
 				}
 
-				return this.resolve(resolved, importer, options);
+				// We are in build mode so return the absolute path to the polyfill.
+				return this.resolve(result.resolved, importer, options);
 			},
 			async transform(code, id) {
 				// Inject the Node.js compat globals into the entry module for Node.js compat environments.
 				const workerConfig = getWorkerConfig(this.environment.name);
-				if (!isNodeCompat(workerConfig)) {
-					return;
-				}
-
+				assert(workerConfig, "Expected a worker config");
 				const resolvedId = await this.resolve(workerConfig.main);
 				if (id === resolvedId?.id) {
 					return injectGlobalCode(id, code);

packages/vite-plugin-cloudflare/src/miniflare-options.ts

@@ -16,7 +16,6 @@ import {
 } from "./constants";
 import { getWorkerConfigPaths } from "./deploy-config";
 import { MODULE_PATTERN } from "./shared";
-import { nodeBuiltInModules } from "./utils";
 import type { CloudflareDevEnvironment } from "./cloudflare-environment";
 import type {
 	PersistState,
@@ -331,9 +330,7 @@ export function getDevMiniflareOptions(
 
 									const shouldExternalize =
 										// Worker modules (CompiledWasm, Text, Data)
-										moduleRE.test(moduleId) ||
-										// Node.js builtin node modules (they will be resolved to unenv aliases)
-										nodeBuiltInModules.has(moduleId);
+										moduleRE.test(moduleId);
 
 									if (shouldExternalize) {
 										const result = {

packages/vite-plugin-cloudflare/src/node-js-compat.ts

@@ -11,8 +11,6 @@ const { env } = defineEnv({
 	presets: [cloudflare],
 });
 
-const CLOUDFLARE_VIRTUAL_PREFIX = "\0__CLOUDFLARE_NODEJS_COMPAT__";
-
 /**
  * Returns true if the given combination of compat dates and flags means that we need Node.js compatibility.
  */
@@ -43,21 +41,47 @@ export function isNodeCompat(
 }
 
 /**
- * If the current environment needs Node.js compatibility,
- * then inject the necessary global polyfills into the code.
+ * Gets a set of module specifiers for all possible Node.js compat polyfill entry-points
+ */
+export function getNodeCompatEntries() {
+	const entries = new Set<string>(Object.values(env.alias));
+	for (const globalInject of Object.values(env.inject)) {
+		if (typeof globalInject === "string") {
+			entries.add(globalInject);
+		} else {
+			assert(
+				globalInject[0] !== undefined,
+				"Expected first element of globalInject to be defined"
+			);
+			entries.add(globalInject[0]);
+		}
+	}
+	for (const external of env.external) {
+		entries.delete(external);
+	}
+	return entries;
+}
+
+/**
+ * Gets the necessary global polyfills to inject into the entry-point of the user's code.
  */
 export function injectGlobalCode(id: string, code: string) {
 	const injectedCode = Object.entries(env.inject)
 		.map(([globalName, globalInject]) => {
 			if (typeof globalInject === "string") {
 				const moduleSpecifier = globalInject;
 				// the mapping is a simple string, indicating a default export, so the string is just the module specifier.
-				return `import var_${globalName} from "${CLOUDFLARE_VIRTUAL_PREFIX}${moduleSpecifier}";\nglobalThis.${globalName} = var_${globalName};\n`;
+				return `import var_${globalName} from "${moduleSpecifier}";\nglobalThis.${globalName} = var_${globalName};\n`;
 			}
 
 			// the mapping is a 2 item tuple, indicating a named export, made up of a module specifier and an export name.
 			const [moduleSpecifier, exportName] = globalInject;
-			return `import var_${globalName} from "${CLOUDFLARE_VIRTUAL_PREFIX}${moduleSpecifier}";\nglobalThis.${globalName} = var_${globalName}.${exportName};\n`;
+			assert(
+				moduleSpecifier !== undefined,
+				"Expected moduleSpecifier to be defined"
+			);
+			assert(exportName !== undefined, "Expected exportName to be defined");
+			return `import var_${globalName} from "${moduleSpecifier}";\nglobalThis.${globalName} = var_${globalName}.${exportName};\n`;
 		})
 		.join("\n");
 
@@ -70,63 +94,29 @@ export function injectGlobalCode(id: string, code: string) {
 }
 
 /**
- * We only want to alias Node.js built-ins if the environment has Node.js compatibility turned on.
- * But Vite only allows us to configure aliases at the shared options level, not per environment.
- * So instead we alias these to a virtual module, which are then handled with environment specific code in the `resolveId` handler
- */
-export function getNodeCompatAliases() {
-	const aliases: Record<string, string> = {};
-	Object.keys(env.alias).forEach((key) => {
-		// Don't create aliases for modules that are already marked as external
-		if (!env.external.includes(key)) {
-			aliases[key] = CLOUDFLARE_VIRTUAL_PREFIX + key;
-		}
-	});
-	return aliases;
-}
-
-/**
- * Get an array of modules that should be considered external.
+ * Gets an array of modules that should be considered external.
  */
 export function getNodeCompatExternals(): string[] {
 	return env.external;
 }
 
 /**
- * If the `source` module id starts with the virtual prefix then strip it and return the rest of the id.
- * Otherwise return undefined.
- */
-export function maybeStripNodeJsVirtualPrefix(
-	source: string
-): string | undefined {
-	return source.startsWith(CLOUDFLARE_VIRTUAL_PREFIX)
-		? source.slice(CLOUDFLARE_VIRTUAL_PREFIX.length)
-		: undefined;
-}
-
-/**
- * Resolve the source import to an absolute path, potentially via its alias.
+ * Resolves the `source` to a Node.js compat alias if possible.
+ *
+ * If there is an alias, the return value is an object with:
+ * - `unresolved`: a bare import path to the polyfill (e.g. `unenv/runtime/node/crypto`)
+ * - `resolved`: an absolute path to the polyfill (e.g. `/path/to/project/node_modules/unenv/runtime/node/child_process/index.mjs`)
  */
 export function resolveNodeJSImport(source: string) {
 	const alias = env.alias[source];
+
+	// These aliases must be resolved from the context of this plugin since the alias will refer to one of the
+	// `@cloudflare/unenv-preset` or the `unenv` packages, which are direct dependencies of this package,
+	// and not the user's project.
 	if (alias) {
-		// If `alias` is `undefined` then `source` was injected in the `transform` hook and we can resolve it directly.
-		// Else we resolve the `alias` instead of the `source`.
-		assert(
-			!env.external.includes(alias),
-			`Unexpected unenv alias to external module: ${source} -> ${alias}`
-		);
-		source = alias;
+		return {
+			unresolved: alias,
+			resolved: resolvePathSync(alias, { url: import.meta.url }),
+		};
 	}
-	// Resolve to an absolute path using the path to this file as the resolution starting point.
-	// This is essential so that we can find the `@cloudflare/unenv-preset` and `unenv` packages,
-	// which are dependencies of this package but may not be direct dependencies of the user's project.
-	const resolved = resolvePathSync(source, {
-		url: import.meta.url,
-	});
-
-	return {
-		unresolved: source,
-		resolved,
-	};
 }

packages/vite-plugin-cloudflare/src/utils.ts

@@ -1,13 +1,8 @@
-import { builtinModules } from "node:module";
 import * as path from "node:path";
 import { Request as MiniflareRequest } from "miniflare";
 import * as vite from "vite";
 import type { IncomingHttpHeaders } from "node:http";
 
-export const nodeBuiltInModules = new Set(
-	builtinModules.concat(builtinModules.map((m) => `node:${m}`))
-);
-
 export function getOutputDirectory(
 	userConfig: vite.UserConfig,
 	environmentName: string