Commit to AsyncLocalStorage (#216)

* commit to use of AsyncLocalStorage and use it to store the env variables

resolves #121

* remove NODE_ENV from generateGlobalJs and move it to esbuild's define field

* run prettier:fix

* rename NODE_ENV to __NODE_ENV__

* remove globalJs replaces as suggested

* fix unit tests

Author: dario-piotrowicz

.changeset/hot-eagles-love.md

@@ -0,0 +1,11 @@
+---
+'@cloudflare/next-on-pages': major
+---
+
+use and rely on AsyncLocalStorage
+
+previously we've been using the node AsyncLocalStorage in a non-breaking way but now we are committing
+to in and using it to store the global env variables as well
+
+this is a breaking change since moving forward all app running using @cloudflare/next-on-pages must
+have the nodejs_compat compatibility flag set (before not all needed that)

src/buildApplication/buildWorkerFile.ts

@@ -6,6 +6,7 @@ import { tmpdir } from 'os';
 import { cliSuccess } from '../cli';
 import { generateGlobalJs } from './generateGlobalJs';
 import type { ProcessedVercelOutput } from './processVercelOutput';
+import { getNodeEnv } from '../utils/getNodeEnv';
 
 /**
  * Construct a record for the build output map.
@@ -28,10 +29,10 @@ export function constructBuildOutputRecord(item: BuildOutputItem) {
 
 	return `{
 				type: ${JSON.stringify(item.type)},
-				entrypoint: AsyncLocalStoragePromise.then(() => import('${item.entrypoint.replace(
+				entrypoint: import('${item.entrypoint.replace(
 					/^\.vercel\/output\/static\/_worker\.js\/__next-on-pages-dist__\//,
 					'./__next-on-pages-dist__/'
-				)}')),
+				)}')
 			}`;
 }
 
@@ -46,12 +47,7 @@ export async function buildWorkerFile(
 
 	await writeFile(
 		functionsFile,
-		`
-		export const AsyncLocalStoragePromise = import('node:async_hooks').then(({ AsyncLocalStorage }) => {
-			globalThis.AsyncLocalStorage = AsyncLocalStorage;
-		}).catch(() => undefined);
-
-		export const __BUILD_OUTPUT__ = {${[...vercelOutput.entries()]
+		`export const __BUILD_OUTPUT__ = {${[...vercelOutput.entries()]
 			.map(([name, item]) => `"${name}": ${constructBuildOutputRecord(item)}`)
 			.join(',')}};`
 	);
@@ -73,9 +69,10 @@ export async function buildWorkerFile(
 		inject: [functionsFile],
 		target: 'es2022',
 		platform: 'neutral',
-		external: ['node:async_hooks', 'node:buffer', './__next-on-pages-dist__/*'],
+		external: ['node:*', './__next-on-pages-dist__/*'],
 		define: {
 			__CONFIG__: JSON.stringify(vercelConfig),
+			__NODE_ENV__: JSON.stringify(getNodeEnv()),
 		},
 		outfile: outputFile,
 		minify,

src/buildApplication/generateGlobalJs.ts

@@ -1,43 +1,23 @@
-import { cliWarn } from '../cli';
-
 /**
  * Generates the javascript content (as a plain string) that deals with the global scope that needs to be
  * added to the built worker.
  *
  * @returns the plain javascript string that should be added at the top of the the _worker.js file
  */
 export function generateGlobalJs(): string {
-	const nodeEnv = getNodeEnv();
-	return `globalThis.process = { env: { NODE_ENV: '${nodeEnv}' } };`;
-}
-
-enum NextJsNodeEnv {
-	PRODUCTION = 'production',
-	DEVELOPMENT = 'development',
-	TEST = 'test',
-}
-
-function getNodeEnv(): string {
-	const processNodeEnv = process.env.NODE_ENV;
-
-	if (!processNodeEnv) {
-		return NextJsNodeEnv.PRODUCTION;
-	}
+	return `
+		import { AsyncLocalStorage } from 'node:async_hooks';
+		globalThis.AsyncLocalStorage = AsyncLocalStorage;
 
-	const nextJsNodeEnvs = Object.values(NextJsNodeEnv);
-	if (!(nextJsNodeEnvs as string[]).includes(processNodeEnv)) {
-		cliWarn(
-			`
-			WARNING:
-			    The current value of the environment variable NODE_ENV is "${processNodeEnv}",
-			    but the supported values are: ${nextJsNodeEnvs
-						.map(env => `"${env}"`)
-						.join(', ')}.
-			    See: https://nextjs.org/docs/basic-features/environment-variables.
-		`,
-			{ spaced: true }
-		);
-	}
+		const __ENV_ALS__ = new AsyncLocalStorage();
 
-	return processNodeEnv;
+		globalThis.process = {
+			env: new Proxy(
+				{},
+				{
+					get: (_, property) => Reflect.get(__ENV_ALS__.getStore(), property),
+					set: (_, property, value) => Reflect.set(__ENV_ALS__.getStore(), property, value),
+			}),
+		};
+	`;
 }

src/utils/getNodeEnv.ts

@@ -0,0 +1,32 @@
+import { cliWarn } from '../cli';
+
+enum NextJsNodeEnv {
+	PRODUCTION = 'production',
+	DEVELOPMENT = 'development',
+	TEST = 'test',
+}
+
+export function getNodeEnv(): string {
+	const processNodeEnv = process.env.NODE_ENV;
+
+	if (!processNodeEnv) {
+		return NextJsNodeEnv.PRODUCTION;
+	}
+
+	const nextJsNodeEnvs = Object.values(NextJsNodeEnv);
+	if (!(nextJsNodeEnvs as string[]).includes(processNodeEnv)) {
+		cliWarn(
+			`
+			WARNING:
+			    The current value of the environment variable NODE_ENV is "${processNodeEnv}",
+			    but the supported values are: ${nextJsNodeEnvs
+						.map(env => `"${env}"`)
+						.join(', ')}.
+			    See: https://nextjs.org/docs/basic-features/environment-variables.
+		`,
+			{ spaced: true }
+		);
+	}
+
+	return processNodeEnv;
+}

templates/_worker.js/index.ts

@@ -1,24 +1,31 @@
 import { handleRequest } from './handleRequest';
 import { adjustRequestForVercel } from './utils';
+import type { AsyncLocalStorage } from 'node:async_hooks';
+
+declare const __NODE_ENV__: string;
 
 declare const __CONFIG__: ProcessedVercelConfig;
 
 declare const __BUILD_OUTPUT__: VercelBuildOutput;
 
+declare const __ENV_ALS__: AsyncLocalStorage<unknown> & {
+	NODE_ENV: string;
+};
+
 export default {
 	async fetch(request, env, ctx) {
-		globalThis.process.env = { ...globalThis.process.env, ...env };
-
-		const adjustedRequest = adjustRequestForVercel(request);
+		return __ENV_ALS__.run({ ...env, NODE_ENV: __NODE_ENV__ }, () => {
+			const adjustedRequest = adjustRequestForVercel(request);
 
-		return handleRequest(
-			{
-				request: adjustedRequest,
-				ctx,
-				assetsFetcher: env.ASSETS,
-			},
-			__CONFIG__,
-			__BUILD_OUTPUT__
-		);
+			return handleRequest(
+				{
+					request: adjustedRequest,
+					ctx,
+					assetsFetcher: env.ASSETS,
+				},
+				__CONFIG__,
+				__BUILD_OUTPUT__
+			);
+		});
 	},
 } as ExportedHandler<{ ASSETS: Fetcher }>;

tests/src/buildApplication/generateGlobalJs.test.ts

@@ -1,44 +1,28 @@
-import { describe, test, expect, vi } from 'vitest';
+import { describe, test, expect } from 'vitest';
 import { generateGlobalJs } from '../../../src/buildApplication/generateGlobalJs';
 
-describe('generateGlobalContext', async () => {
-	test('should default NODE_ENV to "production"', async () => {
-		runWithNodeEnv('', () => {
+describe('generateGlobalJs', async () => {
+	describe('AsyncLocalStorage', () => {
+		test('should make the AsyncLocalStorage globally available', async () => {
 			const globalJs = generateGlobalJs();
-			const expected =
-				"globalThis.process = { env: { NODE_ENV: 'production' } };";
-			expect(globalJs).toBe(expected);
+			expect(globalJs).toContain(
+				'globalThis.AsyncLocalStorage = AsyncLocalStorage'
+			);
 		});
-	});
-
-	['production', 'development', 'test'].forEach(testNodeEnv =>
-		test(`should set the NODE_ENV to ${testNodeEnv} correctly`, async () => {
-			runWithNodeEnv(testNodeEnv, () => {
-				const globalJs = generateGlobalJs();
-				const expected = `globalThis.process = { env: { NODE_ENV: '${testNodeEnv}' } };`;
-				expect(globalJs).toBe(expected);
-			});
-		})
-	);
 
-	test('should set the NODE_ENV to a non-Next.js value correctly but generate a warning', async () => {
-		runWithNodeEnv('non-next-value', () => {
-			const spy = vi.spyOn(console, 'warn').mockImplementation(() => null);
+		test('create an AsyncLocalStorage and set it as a proxy to process.env', async () => {
 			const globalJs = generateGlobalJs();
-			const expected =
-				"globalThis.process = { env: { NODE_ENV: 'non-next-value' } };";
-			expect(globalJs).toBe(expected);
-			expect(spy).toHaveBeenCalledWith(expect.stringContaining('WARNING:'));
+			expect(globalJs).toContain('const __ENV_ALS__ = new AsyncLocalStorage()');
+
+			const proxyRegexMatch = globalJs.match(
+				/globalThis.process = {[\S\s]*Proxy\(([\s\S]+)\)[\s\S]+}/
+			);
+
+			expect(proxyRegexMatch?.length).toBe(2);
+
+			const proxyBody = proxyRegexMatch?.[1];
+			expect(proxyBody).toContain('Reflect.get(__ENV_ALS__.getStore()');
+			expect(proxyBody).toContain('Reflect.set(__ENV_ALS__.getStore()');
 		});
 	});
 });
-
-function runWithNodeEnv<F extends (...args: unknown[]) => void>(
-	value: string,
-	testFn: F
-): void {
-	const oldNodeEnv = process.env.NODE_ENV;
-	process.env.NODE_ENV = value;
-	testFn();
-	process.env.NODE_ENV = oldNodeEnv;
-}

tests/templates/utils/getNodeEnv.test.ts

@@ -0,0 +1,39 @@
+import { describe, test, expect, vi } from 'vitest';
+import { getNodeEnv } from '../../../src/utils/getNodeEnv';
+
+describe('getNodeEnv', () => {
+	test('should default NODE_ENV to "production"', async () => {
+		runWithNodeEnv('', () => {
+			const nodeEnv = getNodeEnv();
+			expect(nodeEnv).toBe('production');
+		});
+	});
+
+	['production', 'development', 'test'].forEach(testNodeEnv =>
+		test(`should set the NODE_ENV to ${testNodeEnv} correctly`, async () => {
+			runWithNodeEnv(testNodeEnv, () => {
+				const nodeEnv = getNodeEnv();
+				expect(nodeEnv).toBe(testNodeEnv);
+			});
+		})
+	);
+
+	test('should set the NODE_ENV to a non-Next.js value correctly but generate a warning', async () => {
+		runWithNodeEnv('non-next-value', () => {
+			const spy = vi.spyOn(console, 'warn').mockImplementation(() => null);
+			const nodeEnv = getNodeEnv();
+			expect(nodeEnv).toBe('non-next-value');
+			expect(spy).toHaveBeenCalledWith(expect.stringContaining('WARNING:'));
+		});
+	});
+});
+
+function runWithNodeEnv<F extends (...args: unknown[]) => void>(
+	value: string,
+	testFn: F
+): void {
+	const oldNodeEnv = process.env.NODE_ENV;
+	process.env.NODE_ENV = value;
+	testFn();
+	process.env.NODE_ENV = oldNodeEnv;
+}