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

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 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
111 changes: 54 additions & 57 deletions src/node/internal/process.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,63 +26,60 @@ export function nextTick(cb: Function, ...args: unknown[]) {
// for the worker are accessible from the env argument passed into the fetch
// handler and have no impact here.

export const env = new Proxy(
{},
{
// Per Node.js rules. process.env values must be coerced to strings.
// When defined using defineProperty, the property descriptor must be writable,
// configurable, and enumerable using just a falsy check. Getters and setters
// are not permitted.
set(obj: object, prop: PropertyKey, value: any) {
return Reflect.set(obj, prop, `${value}`);
},
defineProperty(
obj: object,
prop: PropertyKey,
descriptor: PropertyDescriptor
) {
validateObject(descriptor, 'descriptor', {});
if (Reflect.has(descriptor, 'get') || Reflect.has(descriptor, 'set')) {
throw new ERR_INVALID_ARG_VALUE(
'descriptor',
descriptor,
'process.env value must not have getter/setter'
);
}
if (!descriptor.configurable) {
throw new ERR_INVALID_ARG_VALUE(
'descriptor.configurable',
descriptor,
'process.env value must be configurable'
);
}
if (!descriptor.enumerable) {
throw new ERR_INVALID_ARG_VALUE(
'descriptor.enumerable',
descriptor,
'process.env value must be enumerable'
);
}
if (!descriptor.writable) {
throw new ERR_INVALID_ARG_VALUE(
'descriptor.writable',
descriptor,
'process.env value must be writable'
);
}
if (Reflect.has(descriptor, 'value')) {
Reflect.set(descriptor, 'value', `${descriptor.value}`);
} else {
throw new ERR_INVALID_ARG_VALUE(
'descriptor.value',
descriptor,
'process.env value must be specified explicitly'
);
}
return Reflect.defineProperty(obj, prop, descriptor);
},
}
);
export const env = new Proxy(utilImpl.getEnvObject(), {
// Per Node.js rules. process.env values must be coerced to strings.
// When defined using defineProperty, the property descriptor must be writable,
// configurable, and enumerable using just a falsy check. Getters and setters
// are not permitted.
set(obj: object, prop: PropertyKey, value: any) {
return Reflect.set(obj, prop, `${value}`);
},
defineProperty(
obj: object,
prop: PropertyKey,
descriptor: PropertyDescriptor
) {
validateObject(descriptor, 'descriptor', {});
if (Reflect.has(descriptor, 'get') || Reflect.has(descriptor, 'set')) {
throw new ERR_INVALID_ARG_VALUE(
'descriptor',
descriptor,
'process.env value must not have getter/setter'
);
}
if (!descriptor.configurable) {
throw new ERR_INVALID_ARG_VALUE(
'descriptor.configurable',
descriptor,
'process.env value must be configurable'
);
}
if (!descriptor.enumerable) {
throw new ERR_INVALID_ARG_VALUE(
'descriptor.enumerable',
descriptor,
'process.env value must be enumerable'
);
}
if (!descriptor.writable) {
throw new ERR_INVALID_ARG_VALUE(
'descriptor.writable',
descriptor,
'process.env value must be writable'
);
}
if (Reflect.has(descriptor, 'value')) {
Reflect.set(descriptor, 'value', `${descriptor.value}`);
} else {
throw new ERR_INVALID_ARG_VALUE(
'descriptor.value',
descriptor,
'process.env value must be specified explicitly'
);
}
return Reflect.defineProperty(obj, prop, descriptor);
},
});

export function getBuiltinModule(id: string): any {
return utilImpl.getBuiltinModule(id);
Expand Down
1 change: 1 addition & 0 deletions src/node/internal/util.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ export function isBoxedPrimitive(
value: unknown
): value is number | string | boolean | bigint | symbol;

export function getEnvObject(): Record<string, string>;
export function getBuiltinModule(id: string): any;
export function getCallSite(frames: number): Record<string, string>[];
export function processExitImpl(code: number): void;
Expand Down
20 changes: 20 additions & 0 deletions src/workerd/api/node/tests/process-nodejs-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,23 @@ export const processPlatform = {
assert.ok(['darwin', 'win32', 'linux'].includes(process.platform));
},
};

process.env.BAZ = 1;
const env = { ...process.env };

export const processEnv = {
async test() {
assert.strictEqual(env.FOO, 'BAR');

// It should be possible to mutate the process.env at runtime.
assert.strictEqual(env.BAZ, '1');

// Any binding that is not explicitly a text binding should be
// ignored.
assert.strictEqual(env.BAR, undefined);

// Test that imports can see the process.env at the top level
const { FOO } = await import('mod');
jasnell marked this conversation as resolved.
Show resolved Hide resolved
jasnell marked this conversation as resolved.
Show resolved Hide resolved
assert.strictEqual(FOO, 'BAR');
},
};
14 changes: 11 additions & 3 deletions src/workerd/api/node/tests/process-nodejs-test.wd-test
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,18 @@ const unitTests :Workerd.Config = (
( name = "nodejs-process-test",
worker = (
modules = [
(name = "worker", esModule = embed "process-nodejs-test.js")
(name = "worker", esModule = embed "process-nodejs-test.js"),
(name = "mod", esModule = "export const { FOO } = process.env;")
],
compatibilityDate = "2024-12-28",
compatibilityFlags = [
"nodejs_compat",
"nodejs_compat_populate_process_env"
],
bindings = [
(name = "FOO", text = "BAR"),
(name = "BAR", json = "{}"),
],
compatibilityDate = "2024-10-11",
compatibilityFlags = ["nodejs_compat"],
)
),
],
Expand Down
4 changes: 4 additions & 0 deletions src/workerd/api/node/util.c++
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

what about:
getEnvObject -> getProcessEnvObj
getEnv -> getProcessEnv

}

namespace {
[[noreturn]] void handleProcessExit(jsg::Lock& js, int code) {
// There are a few things happening here. First, we abort the current IoContext
Expand Down
5 changes: 5 additions & 0 deletions src/workerd/api/node/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,8 @@ class UtilModule final: public jsg::Object {
return processPlatform;
}

jsg::JsObject getEnvObject(jsg::Lock& js);

JSG_RESOURCE_TYPE(UtilModule) {
JSG_NESTED_TYPE(MIMEType);
JSG_NESTED_TYPE(MIMEParams);
Expand All @@ -258,6 +260,9 @@ class UtilModule final: public jsg::Object {
JSG_METHOD(previewEntries);
JSG_METHOD(getConstructorName);
JSG_METHOD(getCallSite);
// TODO(cleanup): It might be about time to separate some of these out
// to a different module.
JSG_METHOD(getEnvObject);
jasnell marked this conversation as resolved.
Show resolved Hide resolved

#define V(Type) JSG_METHOD(is##Type);
JS_UTIL_IS_TYPES(V)
Expand Down
6 changes: 6 additions & 0 deletions src/workerd/io/compatibility-date.capnp
Original file line number Diff line number Diff line change
Expand Up @@ -679,4 +679,10 @@ struct CompatibilityFlags @0x8f8c1b68151b6cef {
$compatDisableFlag("cache_no_cache_disabled")
$experimental;
# Enables the use of cache: no-cache in the fetch api.

populateProcessEnv @71 :Bool
$compatEnableFlag("nodejs_compat_populate_process_env")
$compatDisableFlag("nodejs_compat_dot_not_populate_process_env");
jasnell marked this conversation as resolved.
Show resolved Hide resolved
# Automatically populate process.env from text bindings only
# when nodejs_compat is being used.
}
7 changes: 7 additions & 0 deletions src/workerd/jsg/jsg.h
Original file line number Diff line number Diff line change
Expand Up @@ -2675,6 +2675,13 @@ class Lock {
// the inspector (if attached), or to KJ_LOG(Info).
virtual void reportError(const JsValue& value) = 0;

// Sets an env value that will be expressed on the process.env
// if/when nodejs-compat mode is used.
virtual void setEnvField(const JsValue& name, const JsValue& value) = 0;

// Returns the env base object.
virtual JsObject getEnv(bool release = false) = 0;

private:
// Mark the jsg::Lock as being disallowed from being passed as a parameter into
// a kj promise coroutine. Note that this only blocks directly passing the Lock
Expand Down
1 change: 1 addition & 0 deletions src/workerd/jsg/setup.c++
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,7 @@ void IsolateBase::dropWrappers(kj::FunctionParam<void()> drop) {
// Make sure v8::Globals are destroyed under lock (but not until later).
KJ_DEFER(symbolAsyncDispose.Reset());
KJ_DEFER(opaqueTemplate.Reset());
KJ_DEFER(envObj.Reset());

// Make sure the TypeWrapper is destroyed under lock by declaring a new copy of the variable
// that is destroyed before the lock is released.
Expand Down
21 changes: 21 additions & 0 deletions src/workerd/jsg/setup.h
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,9 @@ class IsolateBase {
// object with 2 internal fields.
v8::Global<v8::FunctionTemplate> opaqueTemplate;

// Object that is used as the underlying target of process.env when nodejs-compat mode is used.
v8::Global<v8::Object> envObj;

// Polyfilled Symbol.asyncDispose.
v8::Global<v8::Symbol> symbolAsyncDispose;

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

// Sets an env value that will be expressed on the 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.

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

// if/when nodejs-compat mode is used.
void setEnvField(const JsValue& name, const JsValue& value) override {
getEnv().set(*this, name, value);
}

// Returns the env base object.
JsObject getEnv(bool release = false) override {
KJ_DEFER({
if (release) jsgIsolate.envObj.Reset();
});
if (jsgIsolate.envObj.IsEmpty()) {
v8::Local<v8::Object> env = obj();
jsgIsolate.envObj.Reset(v8Isolate, env);
}
return JsObject(jsgIsolate.envObj.Get(v8Isolate));
}

private:
Isolate& jsgIsolate;

Expand Down
3 changes: 3 additions & 0 deletions src/workerd/server/workerd-api.c++
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@jasnell jasnell Jan 10, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@jasnell jasnell Jan 11, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

}
}

KJ_CASE_ONEOF(data, kj::Array<byte>) {
Expand Down
Loading