Implement importable env #3610

jasnell · 2025-02-26T16:43:24Z

PR marked as a draft until we're certain this is what we want....

import { env } from 'cloudflare:workers';

console.log(env.FOO);  // Undefined, outside request io context

export default {
  fetch(req) {
    console.log(env.FOO);  // 'BAR', inside request io context
    // ...
  }
}

~~Gated by a compat flag that is off by default~~ The flag is now on by default.

allow_importable_env
disallow_importable_env

Mutations of the env properties are passed through to the underlying object so will be reflected on the env argument passed into the handlers.

Note that this is not related to process.env... this exposes the regular env/bindings via the import when accessed within the context of a request. As such, any modules used by your worker can access the bindings without having them explicitly passed down.

jasnell · 2025-02-27T00:28:39Z

Summary of internal discussion:

We want to go ahead and make env also populated at the top-level scope (PR updated to do so)
It has been suggested that we need a way of propagating a modified/custom context only down specific async contexts (PR updated to do so using withEnv(...)). This uses async context propagation under the covers.
Test case has been updated to illustrate the updated functionality.

src/workerd/api/modules.h

src/workerd/io/compatibility-date.capnp

src/workerd/api/global-scope.c++

jasnell · 2025-02-27T01:20:59Z

Next round of updates:

Reversed the polarity of the compat flag. The importable env is now populated/enabled by default with a compat flag to switch it off
withEnv(...) will work even if the disable flag is turned on.

Next known remaining todos:

~~Simplify the instrumentation to avoid having to instrument multiple times. Implement importable env #3610 (comment)~~ Done

src/workerd/api/global-scope.c++

src/workerd/api/modules.h

src/workerd/io/worker.c++

jasnell · 2025-02-27T19:51:00Z

Hmm, yeah, damn you're right :-/ ... ok, it's going to get rather complicated then. The other alternative I was considering is adding a new Worker::current() backed by a thread-local that would allow me to Worker::current().getEnv(js) ... but I'm a bit leery about adding a new thread local. The other approach I considered was adding a callback that can be set such that js.getWorkerEnv() calls back to the worker to grab the env, but I like this less than the thread local. What do you think @kentonv

kentonv · 2025-02-27T19:55:09Z

See my last comment above:

Note that we don't need imported env to support dynamic bindings. For people using dynamic bindings, we can just say: don't use imported env. So I think imported env can just reflect the first env ever received. But the env delivered to event handlers as a parameter still needs to be the per-request env, which we still need to store in Worker::Impl.

So I think you can keep the implementation as-is, except don't remove Worker::Impl::env... there's just two places where we track env, oh well.

vicb · 2025-02-27T22:12:33Z

src/workerd/api/tests/disable-importable-env-test.wd-test

+        ],
+        compatibilityDate = "2025-02-01",
+        compatibilityFlags = [
+          "nodejs_compat_v2",


Does this need nodejs compat?

Oh... Assert

I've been debating whether to make node:assert always available in wd-tests.

vicb · 2025-02-27T22:26:30Z

The PR description mentions that the flag is off by default. I think this is not the current state.

Would be nice to read about security considerations in the description too. IIUC this could be used by any library to get access to the bindings.

vicb · 2025-02-27T22:38:58Z

One last thing: is "(dis)allow_importable_env" really reflecting what this does?

IIUC the PR does not (dis)allow anything. Env is always importable - sometimes not populated/connected.

Would be nice to have a better description on the PR description and maybe a name reflecting the actual behavior.

Thanks

jasnell · 2025-02-27T22:41:12Z

Happy to accept suggestions for alternative names for the compat flags. I feel they are descriptive of what they are doing but opinions may vary

```js import { env } from 'cloudflare:workers'; console.log(env.FOO); // Undefined, outside request io context export default { fetch(req) { console.log(env.FOO); // 'BAR', inside request io context // ... } } ``` Gated by a compat flag that is off by default * `allow_importable_env` * `disallow_importable_env` Mutations of the env properties are passed through to the underlying object so will be reflected on the `env` argument passed into the handlers.

Rather than persisting the env in Worker::Impl, store it in jsg::Lock so that it is easier to access from places that do not have access to Worker::Impl.

vicb · 2025-02-28T06:57:29Z

"Note that this is not related to process.env..."

It actually is somehow related now that process.env is populated from the env.
IMO it's important to document that mutating an importable env will not affect process.env (and probably vice-versa while we're are at it)

vicb · 2025-02-28T08:33:52Z

src/workerd/api/tests/disable-importable-env-test.js

+
+import { strictEqual, notDeepStrictEqual } from 'node:assert';
+
+strictEqual(env.FOO, undefined);


maybe add a test setting env.BAR not reflected on argEnv.BAR

vicb · 2025-02-28T08:35:48Z

src/workerd/io/compatibility-date.capnp

+  # When enabled, `import { env } from 'cloudflare:workers'` will provide access
+  # the per-request environment/bindings.


Suggested change

# When enabled, `import { env } from 'cloudflare:workers'` will provide access

# the per-request environment/bindings.

# When allowed, `import { env } from 'cloudflare:workers'` will provide access

# to the per-request environment/bindings.

vicb · 2025-02-28T12:24:39Z

src/cloudflare/workers.ts

+      if (inner) {
+        return Reflect.deleteProperty(inner, prop);
+      }
+      return true;


Should this return false here ?

jasnell requested review from southpolesteve, kentonv, danlapid and threepointone February 26, 2025 16:43

kentonv reviewed Feb 27, 2025

View reviewed changes

src/workerd/api/modules.h Outdated Show resolved Hide resolved

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

jasnell force-pushed the jasnell/importable-env branch from 8d7dffe to 66d9c5c Compare February 27, 2025 00:35

kentonv reviewed Feb 27, 2025

View reviewed changes

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

jasnell added the needs-internal-pr label Feb 27, 2025

kentonv reviewed Feb 27, 2025

View reviewed changes

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

src/workerd/api/modules.h Outdated Show resolved Hide resolved

src/workerd/io/worker.c++ Outdated Show resolved Hide resolved

jasnell force-pushed the jasnell/importable-env branch from 36e3c6f to 9a88837 Compare February 27, 2025 18:02

kentonv approved these changes Feb 27, 2025

View reviewed changes

jasnell marked this pull request as ready for review February 27, 2025 19:34

jasnell requested review from a team as code owners February 27, 2025 19:34

jasnell requested a review from erikcorry February 27, 2025 19:34

kentonv requested changes Feb 27, 2025

View reviewed changes

src/workerd/io/worker.c++ Outdated Show resolved Hide resolved

jasnell requested a review from kentonv February 27, 2025 21:01

vicb reviewed Feb 27, 2025

View reviewed changes

jasnell added 5 commits February 27, 2025 15:16

Expose populated env at the global scope, expand test

cf7c4c2

Implement withEnv to propagate modified workers env

1e87ae6

Invert the compat flag, make withEnv work even with flag disabled

b654754

Fixup linting issues

ad6a06a

jasnell added 2 commits February 27, 2025 15:16

Simplify instrumentation, store env in jsg::Lock

1633b40

Rather than persisting the env in Worker::Impl, store it in jsg::Lock so that it is easier to access from places that do not have access to Worker::Impl.

Address issue with dynamic bindings

2603d7e

jasnell force-pushed the jasnell/importable-env branch from f07505e to 2603d7e Compare February 27, 2025 23:16

vicb reviewed Feb 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement importable env #3610

Implement importable env #3610

jasnell commented Feb 26, 2025 •

edited

Loading

jasnell commented Feb 27, 2025

jasnell commented Feb 27, 2025 •

edited

Loading

jasnell commented Feb 27, 2025 •

edited

Loading

kentonv commented Feb 27, 2025 •

edited

Loading

vicb Feb 27, 2025

vicb Feb 27, 2025

jasnell Feb 27, 2025

vicb commented Feb 27, 2025

vicb commented Feb 27, 2025 •

edited

Loading

jasnell commented Feb 27, 2025

vicb commented Feb 28, 2025

vicb Feb 28, 2025

vicb Feb 28, 2025 •

edited

Loading

vicb Feb 28, 2025


		import { strictEqual, notDeepStrictEqual } from 'node:assert';

		strictEqual(env.FOO, undefined);

		# When enabled, `import { env } from 'cloudflare:workers'` will provide access
		# the per-request environment/bindings.

Implement importable env #3610

Are you sure you want to change the base?

Implement importable env #3610

Conversation

jasnell commented Feb 26, 2025 • edited Loading

jasnell commented Feb 27, 2025

jasnell commented Feb 27, 2025 • edited Loading

jasnell commented Feb 27, 2025 • edited Loading

kentonv commented Feb 27, 2025 • edited Loading

vicb Feb 27, 2025

Choose a reason for hiding this comment

vicb Feb 27, 2025

Choose a reason for hiding this comment

jasnell Feb 27, 2025

Choose a reason for hiding this comment

vicb commented Feb 27, 2025

vicb commented Feb 27, 2025 • edited Loading

jasnell commented Feb 27, 2025

vicb commented Feb 28, 2025

vicb Feb 28, 2025

Choose a reason for hiding this comment

vicb Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

vicb Feb 28, 2025

Choose a reason for hiding this comment

jasnell commented Feb 26, 2025 •

edited

Loading

jasnell commented Feb 27, 2025 •

edited

Loading

jasnell commented Feb 27, 2025 •

edited

Loading

kentonv commented Feb 27, 2025 •

edited

Loading

vicb commented Feb 27, 2025 •

edited

Loading

vicb Feb 28, 2025 •

edited

Loading