Provide mechanism for autopopulating node.js process.env #3311

jasnell · 2025-01-09T17:01:54Z

Autopopulates the process.env from bindings in local dev. A similar PR will be needed internally to enable it there as it won't be automatic.

src/workerd/server/workerd-api.c++

src/node/internal/util.d.ts

src/workerd/api/node/util.h

Autopopulates the process.env from bindings in local dev. A similar PR will be needed internally to enable it there as it won't be automatic.

vicb

@irvinebroque can we discuss this?

I oppose this change of it exposed secrets to the world via process.env

jasnell · 2025-01-10T17:15:48Z

I oppose this change of it exposed secrets to the world via process.ENV

To be clear... the change puts any TEXT binding on process.env. We have no way of knowing if a a TEXT binding contains something that is considered a secret or not. If tooling insists on adding secrets to the worker configuration using a TEXT binding then this would mean we just cannot populate process.env automatically at all right now because we have zero ability to differentiate if the value of the TEXT binding is secret or not.

src/workerd/io/compatibility-date.capnp

src/workerd/api/node/tests/process-nodejs-test.js

src/workerd/jsg/setup.h

src/workerd/api/node/tests/process-nodejs-test.js

Co-authored-by: Victor Berchet <[email protected]>

kentonv · 2025-01-10T21:32:33Z

src/workerd/server/workerd-api.c++

@@ -711,6 +711,9 @@ static v8::Local<v8::Value> createBindingValue(JsgWorkerdIsolate::Lock& lock,

    KJ_CASE_ONEOF(text, kj::String) {
      value = lock.wrap(context, kj::mv(text));
+      if (featureFlags.getPopulateProcessEnv() && featureFlags.getNodeJsCompat()) {
+        lock.setEnvField(lock.str(global.name), jsg::JsValue(value));


We also need to call this for JSON vars that parse to a plain string, and any other type of variable whose type is a plain string, otherwise we're still piercing the abstraction here.

It might be easier to just give the entire env object over to the proxy, and let the proxy filter for fields which are strings.

We also need to call this for JSON vars that parse to a plain string,..

As discussed.. I'm strongly -1 on special casing JSON fields that parse to a string vs. other js types, largely because it sets up an inconsistency.

It is extremely common in Node.js for env vars to be defined as JSON strings such that node.js code will unconditionally do JSON.parse(process.env.FOO). If sometimes a JSON binding is ignored, sometimes it comes through as a parsed json string, or sometimes it comes through as an encoded JSON string, then this extremely common Node.js pattern breaks. User code would end up having to be changed to inspect the value of process.env.FOO in advance or wrap the JSON.parse(process.env.FOO) in a try/catch, both of which are unlikely when we're talking about ecosystem modules folks are pulling off npm.

Yes, I understand that we have edge cases that represent what should be plaintext text bindings as JSON bindings but that's an implementation detail/quirk/edge case that I don't think we should be optimizing for.

... and any other type of variable whose type is a plain string

Such as?

To be clear, there are no other bindings in workerd-api.c++ whose type is a plain string so it's not clear what you're suggesting here. Given that the internal repo has a different (more expansive) set of bindings possible then sure, the setEnvField(...) may need to be called in more places in the internal PR but for workerd there aren't other binding types whose value is a plaintext string.

process.env should be limited to specifically the things we call "environment variables" and not other types of bindings.

It might be easier to just give the entire env object over to the proxy

I disagree. This mechanism also works for old service worker syntax workers in which the env object is the globalThis. It would make things rather more complicated if we had to special case these options whereas the current implementation keeps things rather simple.

Further, if we did decide to propagate other types of bindings to process.env we can do so in separate PRs. That shouldn't be blocking for this initial PR

Note: wrangler dev/miniflare use JSON bindings for now so the PR will not work as is.

It appears to do so for local development at least. A quick test of wrangler deploy shows that it appropriately uses TEXT bindings for env vars when pushed to production. So the limitation here would appear to be limited to local dev. Should be fixed, yes, but I don't think it's a blocker for merging.

PR to fix wrangler dev so that it matches production and uses TEXT as appropriate cloudflare/workers-sdk#7738

vicb · 2025-01-11T15:01:19Z

src/workerd/api/node/util.c++

@@ -234,6 +234,10 @@ jsg::JsValue UtilModule::getBuiltinModule(jsg::Lock& js, kj::String specifier) {
  return js.undefined();
 }

+jsg::JsObject UtilModule::getEnvObject(jsg::Lock& js) {
+  return js.getEnv(true);


what about:
getEnvObject -> getProcessEnvObj
getEnv -> getProcessEnv

src/workerd/io/compatibility-date.capnp

vicb · 2025-01-11T15:05:28Z

src/workerd/jsg/setup.h

@@ -665,6 +668,24 @@ class Isolate: public IsolateBase {
      }
    }

+    // Sets an env value that will be expressed on the process.env


expressed -> exposed?
maybe expand on "when nodejs-compat mode is used" (to refer to the flag)

jasnell requested review from irvinebroque, anonrig and danlapid January 9, 2025 17:01

jasnell requested review from a team as code owners January 9, 2025 17:01

jasnell force-pushed the jasnell/workerd-autopopulate-process-env branch 3 times, most recently from 122a5b8 to d08499e Compare January 9, 2025 21:36

kentonv reviewed Jan 9, 2025

View reviewed changes

src/workerd/server/workerd-api.c++ Outdated Show resolved Hide resolved

anonrig reviewed Jan 10, 2025

View reviewed changes

src/node/internal/util.d.ts Outdated Show resolved Hide resolved

src/workerd/api/node/util.h Show resolved Hide resolved

jasnell force-pushed the jasnell/workerd-autopopulate-process-env branch 2 times, most recently from 00c74b2 to f6f3f64 Compare January 10, 2025 15:12

jasnell requested a review from anonrig January 10, 2025 15:15

jasnell force-pushed the jasnell/workerd-autopopulate-process-env branch from f6f3f64 to 7f531ca Compare January 10, 2025 15:15

danlapid approved these changes Jan 10, 2025

View reviewed changes

jasnell force-pushed the jasnell/workerd-autopopulate-process-env branch from 7f531ca to 5356dc7 Compare January 10, 2025 15:54

anonrig approved these changes Jan 10, 2025

View reviewed changes

jasnell commented Jan 10, 2025

View reviewed changes

src/workerd/api/node/util.h Outdated Show resolved Hide resolved

Provide mechanism for autopopulating node.js process.env

09a9009

Autopopulates the process.env from bindings in local dev. A similar PR will be needed internally to enable it there as it won't be automatic.

jasnell force-pushed the jasnell/workerd-autopopulate-process-env branch from e6519ab to 09a9009 Compare January 10, 2025 16:28

vicb suggested changes Jan 10, 2025

View reviewed changes

vicb reviewed Jan 10, 2025

View reviewed changes

src/workerd/io/compatibility-date.capnp Outdated Show resolved Hide resolved

vicb reviewed Jan 10, 2025

View reviewed changes

src/workerd/api/node/tests/process-nodejs-test.js Show resolved Hide resolved

vicb reviewed Jan 10, 2025

View reviewed changes

src/workerd/jsg/setup.h Outdated Show resolved Hide resolved

jasnell commented Jan 10, 2025

View reviewed changes

src/workerd/api/node/tests/process-nodejs-test.js Show resolved Hide resolved

jasnell and others added 2 commits January 10, 2025 11:31

Update src/workerd/jsg/setup.h

ae7d239

Apply suggestions from code review

94cc609

Co-authored-by: Victor Berchet <[email protected]>

kentonv reviewed Jan 10, 2025

View reviewed changes

vicb reviewed Jan 11, 2025

View reviewed changes

src/workerd/io/compatibility-date.capnp Outdated Show resolved Hide resolved

vicb reviewed Jan 11, 2025

View reviewed changes

Update src/workerd/io/compatibility-date.capnp

8ba527b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide mechanism for autopopulating node.js process.env #3311

Provide mechanism for autopopulating node.js process.env #3311

jasnell commented Jan 9, 2025

vicb left a comment •

edited

Loading

jasnell commented Jan 10, 2025

kentonv Jan 10, 2025

kentonv Jan 10, 2025

jasnell Jan 10, 2025 •

edited

Loading

jasnell Jan 10, 2025

vicb Jan 11, 2025

jasnell Jan 11, 2025 •

edited

Loading

jasnell Jan 11, 2025

vicb Jan 11, 2025

vicb Jan 11, 2025

Provide mechanism for autopopulating node.js process.env #3311

Are you sure you want to change the base?

Provide mechanism for autopopulating node.js process.env #3311

Conversation

jasnell commented Jan 9, 2025

vicb left a comment • edited Loading

Choose a reason for hiding this comment

jasnell commented Jan 10, 2025

kentonv Jan 10, 2025

Choose a reason for hiding this comment

kentonv Jan 10, 2025

Choose a reason for hiding this comment

jasnell Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

jasnell Jan 10, 2025

Choose a reason for hiding this comment

vicb Jan 11, 2025

Choose a reason for hiding this comment

jasnell Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

jasnell Jan 11, 2025

Choose a reason for hiding this comment

vicb Jan 11, 2025

Choose a reason for hiding this comment

vicb Jan 11, 2025

Choose a reason for hiding this comment

vicb left a comment •

edited

Loading

jasnell Jan 10, 2025 •

edited

Loading

jasnell Jan 11, 2025 •

edited

Loading