-
Notifications
You must be signed in to change notification settings - Fork 73
refactor: use ALS for process.env
object
#98
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
Changes from all commits
335fac5
627d10b
e7f9206
4be38fa
4628b62
9ecefdf
d3a2bde
9f13e52
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. |
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>) { | ||
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), | ||
} | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export * from "./create-als-proxy"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
// @ts-expect-error - populated when we run inside the ALS context | ||
globalThis.process.env = createALSProxy(processEnvALS); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I agree with @vicb to some extent here. E.g.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ?? "{}"); | ||
|
@@ -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 }>; | ||
|
Uh oh!
There was an error while loading. Please reload this page.