Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: use ALS for process.env object #98

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/funny-impalas-attend.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@opennextjs/cloudflare": patch
---

refactor: use ALS for `process.env` object.

The adaptor was previously manipulating the global process.env object on every request, without accounting for other requests. ALS has been introduced to change this behavior, so that each process.env object is scoped to the request.
18 changes: 18 additions & 0 deletions packages/cloudflare/src/cli/templates/utils/create-als-proxy.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import type { AsyncLocalStorage } from "node:async_hooks";

/**
* Creates a proxy that exposes values from an AsyncLocalStorage store
*
* @param als AsyncLocalStorage instance
*/
export function createALSProxy<T>(als: AsyncLocalStorage<T>) {
james-elicx marked this conversation as resolved.
Show resolved Hide resolved
return new Proxy(
{},
{
ownKeys: () => Reflect.ownKeys(als.getStore()!),
getOwnPropertyDescriptor: (_, ...args) => Reflect.getOwnPropertyDescriptor(als.getStore()!, ...args),
get: (_, property) => Reflect.get(als.getStore()!, property),
set: (_, property, value) => Reflect.set(als.getStore()!, property, value),
}
);
}
1 change: 1 addition & 0 deletions packages/cloudflare/src/cli/templates/utils/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from "./create-als-proxy";
74 changes: 36 additions & 38 deletions packages/cloudflare/src/cli/templates/worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,23 +9,20 @@ import { MockedResponse } from "next/dist/server/lib/mock-request";
import type { NodeRequestHandler } from "next/dist/server/next-server";

import type { CloudflareContext } from "../../api";
import { createALSProxy } from "./utils";

const NON_BODY_RESPONSES = new Set([101, 204, 205, 304]);

const processEnvALS = new AsyncLocalStorage<Record<string, unknown>>();
const cloudflareContextALS = new AsyncLocalStorage<CloudflareContext>();

// Note: this symbol needs to be kept in sync with the one defined in `src/api/get-cloudflare-context.ts`
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(globalThis as any)[Symbol.for("__cloudflare-context__")] = new Proxy(
{},
{
ownKeys: () => Reflect.ownKeys(cloudflareContextALS.getStore()!),
getOwnPropertyDescriptor: (_, ...args) =>
Reflect.getOwnPropertyDescriptor(cloudflareContextALS.getStore()!, ...args),
get: (_, property) => Reflect.get(cloudflareContextALS.getStore()!, property),
set: (_, property, value) => Reflect.set(cloudflareContextALS.getStore()!, property, value),
}
);
(globalThis as any)[Symbol.for("__cloudflare-context__")] = createALSProxy(cloudflareContextALS);

const originalEnv: Partial<typeof process.env> = { ...globalThis.process.env };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to clone this object here? We make a copy of the originalEnv when setting up the ALS.

// @ts-expect-error - populated when we run inside the ALS context
globalThis.process.env = createALSProxy(processEnvALS);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I agree with @vicb to some extent here.
Before any request, there might already be values on process.env, which are being lost by assigning it here.
How about we capture the original process.env somehow and then include that in the initialization of the store below.

E.g.

const originalEnv = globalThis.process.env;
globalThis.process.env = createALSProxy(processEnvALS);
...
export default {
  async fetch(request, env, ctx) {
    return processEnvALS.run({ NODE_ENV: "production", ...originalEnv, ...env }, () => {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to add tests.

(I think we should requires them in the PR template, as well as repro on issue templat - well we have to add those templates first, I can take care of that next week)


// Injected at build time
const nextConfig: NextConfig = JSON.parse(process.env.__NEXT_PRIVATE_STANDALONE_CONFIG ?? "{}");
Expand All @@ -34,40 +31,41 @@ let requestHandler: NodeRequestHandler | null = null;

export default {
async fetch(request, env, ctx) {
return cloudflareContextALS.run({ env, ctx, cf: request.cf }, async () => {
if (requestHandler == null) {
globalThis.process.env = { ...globalThis.process.env, ...env };
// Note: "next/dist/server/next-server" is a cjs module so we have to `require` it not to confuse esbuild
// (since esbuild can run in projects with different module resolutions)
// eslint-disable-next-line @typescript-eslint/no-require-imports
const NextNodeServer = require("next/dist/server/next-server")
.default as typeof import("next/dist/server/next-server").default;

requestHandler = new NextNodeServer({
conf: nextConfig,
customServer: false,
dev: false,
dir: "",
minimalMode: false,
}).getRequestHandler();
}
return processEnvALS.run({ NODE_ENV: "production", ...originalEnv, ...env }, () => {
return cloudflareContextALS.run({ env, ctx, cf: request.cf }, async () => {
if (requestHandler == null) {
// Note: "next/dist/server/next-server" is a cjs module so we have to `require` it not to confuse esbuild
// (since esbuild can run in projects with different module resolutions)
// eslint-disable-next-line @typescript-eslint/no-require-imports
const NextNodeServer = require("next/dist/server/next-server")
.default as typeof import("next/dist/server/next-server").default;

requestHandler = new NextNodeServer({
conf: nextConfig,
customServer: false,
dev: false,
dir: "",
minimalMode: false,
}).getRequestHandler();
}

const url = new URL(request.url);
const url = new URL(request.url);

if (url.pathname === "/_next/image") {
const imageUrl =
url.searchParams.get("url") ?? "https://developers.cloudflare.com/_astro/logo.BU9hiExz.svg";
if (imageUrl.startsWith("/")) {
return env.ASSETS.fetch(new URL(imageUrl, request.url));
if (url.pathname === "/_next/image") {
const imageUrl =
url.searchParams.get("url") ?? "https://developers.cloudflare.com/_astro/logo.BU9hiExz.svg";
if (imageUrl.startsWith("/")) {
return env.ASSETS.fetch(new URL(imageUrl, request.url));
}
return fetch(imageUrl, { cf: { cacheEverything: true } });
}
return fetch(imageUrl, { cf: { cacheEverything: true } });
}

const { req, res, webResponse } = getWrappedStreams(request, ctx);
const { req, res, webResponse } = getWrappedStreams(request, ctx);

ctx.waitUntil(Promise.resolve(requestHandler(new NodeNextRequest(req), new NodeNextResponse(res))));
ctx.waitUntil(Promise.resolve(requestHandler(new NodeNextRequest(req), new NodeNextResponse(res))));

return await webResponse();
return await webResponse();
});
});
},
} as ExportedHandler<{ ASSETS: Fetcher }>;
Expand Down