refactor: use ALS for process.env object
#98
Conversation
🦋 Changeset detectedLatest commit: 9f13e52 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
james-elicx
changed the title
commit: |
packages/cloudflare/src/cli/templates/utils/create-als-proxy.ts
Outdated
Show resolved
Hide resolved
packages/cloudflare/src/cli/templates/utils/create-als-proxy.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Dario Piotrowicz <[email protected]>
james-elicx
force-pushed
the
james/process-env-als
branch
from
0d86245 to
e7f9206
Compare
james-elicx
changed the title
process object
This reverts commit 4be38fa.
james-elicx
force-pushed
the
james/process-env-als
branch
from
a3a98a8 to
9ecefdf
Compare
james-elicx
changed the title
process objectprocess.env object
There was a problem hiding this comment.
Looks good to me, thanks for the improvement 🙂
@james-elicx if you would could we wait for @petebacondarwin to maybe have a quick look tomorrow? Since @vicb seemed against the change I think it'd be great to have also Pete's point of view on this one 🙂
| (globalThis as any)[Symbol.for("__cloudflare-context__")] = createALSProxy(cloudflareContextALS); | ||
|
|
||
| // @ts-expect-error - populated when we run inside the ALS context | ||
| globalThis.process.env = createALSProxy(processEnvALS); |
There was a problem hiding this comment.
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 }, () => {
There was a problem hiding this comment.
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)
| @@ -20,6 +20,7 @@ const cloudflareContextALS = new AsyncLocalStorage<CloudflareContext>(); | |||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
| (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.
Do we need to clone this object here? We make a copy of the originalEnv when setting up the ALS.
Context
process.env is currently assigned via spreading env when there is no request handler. The env values available should be scoped to a request, and dealt with on a per-request basis.
Changes
I recommend hiding whitespace changes when looking at the diff in this pr.