unjs / unenv

๐Ÿ•Š๏ธ Convert javaScript code to be runtime agnostic
MIT License
342 stars 18 forks source link

fix(node): rewrite the process.env polyfill #186

Closed IgorMinar closed 1 month ago

IgorMinar commented 1 month ago

๐Ÿ”— Linked issue

https://github.com/unjs/unenv/pull/183

โ“ Type of change

๐Ÿ“š Description

The previous polyfill was unnecessarily complicated and hard to understand. Additionally it also used fallback on globalThis to read and write env variables, which in the past was used to read env variables in Cloudflare Workers in the case where workers were using the legacy service worker format. This style of workers is not actively used any more so this fallback is not needed, and in fact is undesirable as we never want to accidentaly write this globalThis.foo as a result of an assignment to process.env.foo.

The nitro utilized globalThis.env fallback is preserved as to no break nitro users.

๐Ÿ“ Checklist

pi0 commented 1 month ago

I appreciate your attempt to improve this but it probably won't work as expected anymore.

For cloudflare presets, we assign to globalThis.__env__ after first fetch event. It is after when the polyfill being initialized and the ref to globalThis.__env__ (undefined) at time of startup won't be updated to new process.env.

Besides, since the polyfill needs to cover other runtimes such as Deno.env which has explicit getters and setters proxy will be needed for them.

(And to add to the complexity, there are frameworks such as astro use import.meta.env.* so we cannot really simplify env support that much)

I think simplest way to disable legacy cloudflare-service-worker behavior that picks the globalThis.* would be to assign an empty object to globalThis.__env__ to disable current behavior.


We can introduce a trimmed-down polyfill via polyfill/cloudflare.ts being introduced in #183 if you like but still we need a getter method at least to allow injection after fetch.

IgorMinar commented 1 month ago

Interesting. Thanks for the explanation @pi0. Let me think about this a bit more.

IgorMinar commented 1 month ago

I just noticed this line of code: https://github.com/unjs/unenv/blob/e837dfab520d0e31a259e7868509ac40478a614e/src/runtime/node/process/_env.ts#L2

which when combined with process.getBuiltinModule from https://github.com/cloudflare/workerd/pull/2147 should make it unnecessary for us to make any changes with unenv.

I'm going to close this PR for now and we can revisit it later.

I noticed that https://github.com/unjs/unenv/pull/183 was merged, which I'm not very keen on, and I'll start a convo on discord about considering to revert it.