implement route scoping solution

.changeset/thirty-birds-build.md

@@ -0,0 +1,30 @@
+---
+'@cloudflare/next-on-pages': patch
+---
+
+fix: implement route specific global scoping strategy
+
+currently routes all share the same global scope, this can be problematic and cause
+race conditions and failures
+
+One example of this is the following code that is present in route function files:
+
+```ts
+self.webpackChunk_N_E = ...
+```
+
+and
+
+```ts
+self.webpackChunk_N_E.push(...)
+```
+
+this indicates that an in-memory global collection of the webpack chunks is shared by all routes,
+this combined with the fact that chunks can have their own module state this can easily cause routes to conflict with each other at runtime.
+
+So, in order to solve the above issue, all route functions are wrapped in a function which accepts as parameters, thus overrides, the `self`, `globalThis` and `global` symbols. The symbols
+will be resolved with proxies that redirect setters to route-scoped in-memory maps and
+getters to the above mentioned map's values and fallback to the original symbol values otherwise
+(i.e. `globalThis` will be overridden by a proxy that, when setting values, sets them in a separate
+location and, when getting values, gets them from said location if present there or from the real
+`globalThis` otherwise)

packages/next-on-pages/src/buildApplication/processVercelFunctions/dedupeEdgeFunctions.ts

@@ -101,13 +101,20 @@ async function processFunctionIdentifiers(
 
 			if (importPath) {
 				// Dedupe and update collected imports.
-				const { updatedContents } = await processImportIdentifier(
-					{ type, identifier, start, end, importPath, info: identifierInfo },
-					{ fileContents, entrypoint, newFnLocation, fnConfig: fnInfo.config },
-					opts,
-				);
+				const { updatedContents, newImportToPrepend } =
+					await processImportIdentifier(
+						{ type, identifier, start, end, importPath, info: identifierInfo },
+						{
+							fileContents,
+							entrypoint,
+							newFnLocation,
+							fnConfig: fnInfo.config,
+						},
+						opts,
+					);
 
 				fileContents = updatedContents;
+				importsToPrepend.push(newImportToPrepend);
 			} else if (identifierInfo.consumers.length > 1) {
 				// Only dedupe code blocks if there are multiple consumers.
 				const { updatedContents, newFilePath, newImport, wasmImports } =
@@ -143,9 +150,10 @@ async function processFunctionIdentifiers(
 
 		// Build the identifier files before building the function's file.
 		await Promise.all(
-			[...identifierPathsToBuild].map(async path =>
-				buildFile(await readFile(path, 'utf8'), path),
-			),
+			[...identifierPathsToBuild].map(async path => {
+				const fileContents = await functionifyFileContent(path);
+				return buildFile(fileContents, path);
+			}),
 		);
 
 		// If wasm identifier is used in code block, prepend the import to the code block's file.
@@ -168,6 +176,30 @@ async function processFunctionIdentifiers(
 	await Promise.all(functionBuildPromises);
 }
 
+/**
+ * Given a standard ESM file (without imports) it converts it to a function call that returns
+ * an object with the various exports set as its fields
+ *
+ * The function allows us to override global symbols such as `self`, `globalThis` and `global`
+ * (which are used as the function's parameter names)
+ *
+ * @param path the path of the ESM file
+ * @returns the converted file content
+ */
+async function functionifyFileContent(path: string) {
+	const originalFileContents = await readFile(path, 'utf8');
+	return `
+		const namedExports = {};
+		export const getNamedExports = ((self, globalThis, global) => {
+			${originalFileContents.replace(
+				/export\s+const\s+(\S+)\s*=/g,
+				'namedExports["$1"] =',
+			)}
+			return namedExports;
+		});
+	`;
+}
+
 /**
  * Builds a new file for an Edge function.
  *
@@ -193,21 +225,41 @@ async function buildFunctionFile(
 		return acc;
 	}, new Map<string, string>());
 
-	groupedImports.forEach((keys, path) => {
-		const relativeImportPath = getRelativePathToAncestor({
-			from: newFnLocation,
-			relativeTo: nopDistDir,
-		});
+	let chunkMapIdx = 0;
+	const chunksExportsMap = new Map<string, Set<string>>();
+
+	const relativeImportPath = getRelativePathToAncestor({
+		from: newFnLocation,
+		relativeTo: nopDistDir,
+	});
+
+	groupedImports.forEach((exports, path) => {
 		const importPath = normalizePath(
 			join(relativeImportPath, addLeadingSlash(path)),
 		);
 
-		functionImports += `import { ${keys} } from '${importPath}';\n`;
+		if (path.endsWith('.wasm')) {
+			// if we're dealing with a wasm file there is a single default export to deal with
+			const defaultExport = exports;
+			// we don't need/want to apply any code transformation for wasm imports
+			functionImports += `import ${defaultExport} from "${path}";`;
+			return;
+		}
+
+		const namedExportsId = `getNamedExports_${chunkMapIdx++}`;
+		const exportKeys = exports.split(',');
+		chunksExportsMap.set(namedExportsId, new Set(exportKeys));
+		functionImports += `import { getNamedExports as ${namedExportsId} } from '${importPath}';\n`;
 	});
 
 	fnInfo.outputPath = relative(workerJsDir, newFnPath);
 
-	const finalFileContents = `${functionImports}${fileContents}`;
+	const finalFileContents = iffefyFunctionFile(
+		fileContents,
+		functionImports,
+		fnInfo,
+		chunksExportsMap,
+	);
 	const buildPromise = buildFile(finalFileContents, newFnPath, {
 		relativeTo: nopDistDir,
 	}).then(async () => {
@@ -225,6 +277,53 @@ type BuildFunctionFileOpts = {
 	newFnPath: string;
 };
 
+/**
+ * Given the content of a function file it converts/wraps it into an iife that overrides the function's contents with an iffe call that
+ * overrides global symbols with route-specific proxies (for more details see: templates/_worker.js/routesIsolation.ts)
+ *
+ * @param fileContents the function file's contents
+ * @param functionImports the imports that need to be added to the file
+ * @param fnInfo the function's information
+ * @param chunksExportsMap a map containing getters and chunks identifiers being used by the function
+ * @returns the updated/iifefied file content
+ */
+function iffefyFunctionFile(
+	fileContents: string,
+	functionImports: string,
+	fnInfo: FunctionInfo,
+	chunksExportsMap: Map<string, Set<string>>,
+): string {
+	const wrappedContent = `
+		export default ((self, globalThis, global) => {
+			${fileContents
+				// it looks like there can be direct references to _ENTRIES (i.e. `_ENTRIES` instead of `globalThis._ENTRIES` etc...)
+				// we have to update all such references otherwise our proxying won't take effect on those
+				.replace(/([^.])_ENTRIES/g, '$1globalThis._ENTRIES')
+				// the default export needs to become the return value of the iife, which is then re-exported as default
+				.replace(/export\s+default\s+/g, 'return ')}
+		})(proxy, proxy, proxy);
+	`;
+
+	const proxyCall = `const proxy = globalThis.__nextOnPagesRoutesIsolation.getProxyFor('${
+		fnInfo.route?.path ?? ''
+	}');`;
+
+	const chunksExtraction = [...chunksExportsMap.entries()].flatMap(
+		([getNamedExportsId, keys]) => {
+			return [
+				`const exportsOf${getNamedExportsId} = ${getNamedExportsId}(proxy, proxy, proxy);`,
+				...[...keys.entries()].map(
+					key => `const ${key} = exportsOf${getNamedExportsId}["${key}"]`,
+				),
+			];
+		},
+	);
+
+	return [functionImports, proxyCall, ...chunksExtraction, wrappedContent].join(
+		';',
+	);
+}
+
 /**
  * Prepends Wasm imports to a code block's built file.
  *
@@ -268,12 +367,13 @@ async function prependWasmImportsToCodeBlocks(
  * Processes an import path identifier.
  *
  * - Moves the imported file to the new location if it doesn't exist.
- * - Updates the file contents to import from the new location.
+ * - Updates the file contents to remove the import.
+ * - Returns the information for the new import.
  *
  * @param ident The import path identifier to process.
  * @param processOpts Contents of the function's file, the function's entrypoint, and the new path.
  * @param opts Options for processing the function.
- * @returns The updated file contents.
+ * @returns The updated file contents alongside the new import information.
  */
 async function processImportIdentifier(
 	ident: RawIdentifierWithImport<IdentifierType> & { info: IdentifierInfo },
@@ -284,7 +384,7 @@ async function processImportIdentifier(
 		fnConfig,
 	}: ProcessImportIdentifierOpts,
 	{ nopDistDir, workerJsDir }: ProcessVercelFunctionsOpts,
-): Promise<{ updatedContents: string }> {
+): Promise<{ updatedContents: string; newImportToPrepend: NewImportInfo }> {
 	const { type, identifier, start, end, importPath, info } = ident;
 	let updatedContents = fileContents;
 
@@ -320,14 +420,20 @@ async function processImportIdentifier(
 	});
 	const newImportPath = normalizePath(join(relativeImportPath, info.newDest));
 
-	const newVal = `import ${identifier} from "${newImportPath}";`;
+	// let's remove the original import since it will be re-added later when appropriate
 	updatedContents = replaceLastSubstringInstance(
 		updatedContents,
 		codeBlock,
-		newVal,
+		'',
 	);
 
-	return { updatedContents };
+	return {
+		updatedContents,
+		newImportToPrepend: {
+			key: identifier,
+			path: newImportPath,
+		},
+	};
 }
 
 type ProcessImportIdentifierOpts = {

packages/next-on-pages/templates/_worker.js/index.ts

@@ -1,5 +1,6 @@
 import { SUSPENSE_CACHE_URL } from '../cache';
 import { handleRequest } from './handleRequest';
+import { setupRoutesIsolation } from './routesIsolation';
 import {
 	adjustRequestForVercel,
 	handleImageResizingRequest,
@@ -22,6 +23,7 @@ declare const __ALSes_PROMISE__: Promise<null | {
 
 export default {
 	async fetch(request, env, ctx) {
+		setupRoutesIsolation();
 		patchFetch();
 
 		const asyncLocalStorages = await __ALSes_PROMISE__;

packages/next-on-pages/templates/_worker.js/routesIsolation.ts

@@ -0,0 +1,91 @@
+/**
+ * The next-on-pages worker needs to isolate the global scope for each route, they all sharing the same global scope
+ * allows for race conditions and incorrect behaviors (see: https://github.com/cloudflare/next-on-pages/issues/805)
+ *
+ * So we set up an isolation system in which each route can access a proxy to the global scope that is route-specific,
+ * they can do that by calling `globalThis.getProxyFor(route)`.
+ *
+ * The following function sets up such utility alongside a map that is used to store the various proxies.
+ */
+export function setupRoutesIsolation() {
+	globalThis.__nextOnPagesRoutesIsolation ??= {
+		_map: new Map(),
+		getProxyFor,
+	};
+}
+
+/**
+ * Utility to retrieve a route-specific proxy to the global scope, if the proxy doesn't yet exist it gets created
+ * by the function.
+ *
+ * @param route the target route
+ * @returns the proxy for the route
+ */
+function getProxyFor(route: string) {
+	const existingProxy = globalThis.__nextOnPagesRoutesIsolation._map.get(route);
+	if (existingProxy) {
+		return existingProxy;
+	}
+
+	const newProxy = createNewRouteProxy();
+	globalThis.__nextOnPagesRoutesIsolation._map.set(route, newProxy);
+	return newProxy;
+}
+
+/**
+ * Creates a new route-specific proxy to the global scope.
+ *
+ * How the proxy works: setters on the proxy don't set the values to the actual global scope but
+ * in an internal map specific to the proxy. getters retrieve values from such internal map, and
+ * fall back to the actual global scope for values not present in such map.
+ *
+ * This makes it so that routes trying to modify the global scope will, though this proxy, work
+ * exactly like if they were actually updating the global scope, but without actually doing so,
+ * thus not effecting any other route.
+ *
+ * Note: this does not account for routes trying to update already existing objects in the global
+ * scope (e.g. `globalScope.existing_field.x = 123`), fortunately such granular control doesn't
+ * seem necessary in next-on-pages.
+ *
+ * @returns the created proxy.
+ */
+function createNewRouteProxy() {
+	const overrides = new Map<string | symbol, unknown>();
+
+	return new Proxy(globalThis, {
+		get: (_, property) => {
+			if (overrides.has(property)) {
+				return overrides.get(property);
+			}
+			return Reflect.get(globalThis, property);
+		},
+		set: (_, property, value) => {
+			if (sharedGlobalProperties.has(property)) {
+				// this property should be shared across all routes
+				return Reflect.set(globalThis, property, value);
+			}
+			overrides.set(property, value);
+			return true;
+		},
+	});
+}
+
+/**
+ * There are some properties that do need to be shared across all different routes, so we collect
+ * them in this set and skip the global scope proxying for them
+ */
+const sharedGlobalProperties = new Set<string | symbol>([
+	'_nextOriginalFetch',
+	'fetch',
+	'__incrementalCache',
+]);
+
+type RoutesIsolation = {
+	_map: Map<string, unknown>;
+	getProxyFor: (route: string) => unknown;
+};
+
+declare global {
+	// eslint-disable-next-line no-var
+	var __nextOnPagesRoutesIsolation: RoutesIsolation;
+}

pages-e2e/features/_utils/getAssertVisible.ts

@@ -19,7 +19,6 @@ async function assertVisible(
 	page: Page,
 	...[selector, options]: Parameters<Page['locator']>
 ): Promise<Locator | never> {
-	let isVisible = false;
 	for (const _attempt of [0, 1, 2, 3, 4, 5]) {
 		const locator = page.locator(selector, options);
 		try {

pages-e2e/fixtures/app14.0.0/main.fixture

@@ -4,7 +4,6 @@
 		"appRouting",
 		"appConfigsTrailingSlashTrue",
 		"appWasm",
-		"appServerActions",
 		"appGetRequestContext"
 	],
 	"localSetup": "npm install",