unjs / unenv

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

fix(cloudflare): make sure not fallback to `globalThis` for env #183

Closed jculvey closed 1 month ago

jculvey commented 1 month ago

๐Ÿ”— Linked issue

โ“ Type of change

๐Ÿ“š Description

๐Ÿ“ Checklist

Adds a polyfill for process to the Cloudflare preset which combines the node polyfill with the workerd polyfill implementation. We'd specifically like to retain the polyfilled nextTick from workerd which uses queueMicrotask, but benefit from all the other global process polyfills provided by unenv.

pi0 commented 1 month ago

Can you please update the polyfill to explictly override nextTick impl?

(env implementation is more generic in unenv. we can explicitly add any more override in the future that needed)

Also would it make sense to have a generic polyfills/cloudflare file that adds more of future polyfills via one entry?

jculvey commented 1 month ago

Can you please update the polyfill to explictly override nextTick impl?

(env implementation is more generic in unenv. we can explicitly add any more override in the future that needed)

Also would it make sense to have a generic polyfills/cloudflare file that adds more of future polyfills via one entry?

Great suggestions, updated!

Thanks for the speedy response time ๐Ÿ˜„.

pi0 commented 1 month ago

Created https://github.com/unjs/unenv/pull/184 for nextTick ~> queueMicrotask as general improvement.


On this PR, I would suggest to introduce runtime/polyfill/cloudflare.ts mainly to set __env__ shim override (to solve glboalThis concern of process.env https://github.com/unjs/unenv/pull/183#discussion_r1602104944) and wait for process.getBuiltinModule to be landing before trying the mixed method.


Using node:process import idea, is probably not going to work well in practice because either the unenv cloudflare preset should configure bundler to alias node:process (to unenv) or use external one. If it is aliased to unenv, it will make a recursive situation and if it is external unenv is not effective.

IgorMinar commented 1 month ago

On this PR, I would suggest to introduce runtime/polyfill/cloudflare.ts mainly to set env shim override (to solve glboalThis concern of process.env https://github.com/unjs/unenv/pull/183#discussion_r1602104944) and wait for https://github.com/unjs/unenv/issues/182 to be landing before trying the mixed method.

I addressed this in https://github.com/unjs/unenv/pull/183#discussion_r1602197176

Using node:process import idea, is probably not going to work well in practice because either the unenv cloudflare preset should configure bundler to alias node:process (to unenv) or use external one. If it is aliased to unenv, it will make a recursive situation and if it is external unenv is not effective.

if we resolve the process.env issue as suggested above, we might not need to import node:process at all, but if we do, we would only use it for APIs that are natively implemented in workerd, which will remain exposed once we implement the hybrid polyfills as described in https://github.com/unjs/unenv/issues/181

so I think the last remaining thing here is to resolve the two issues I broght up with the current process.env polyfill (1/ removal of globalThis fallback 2/ swapping the order in read operations).

jculvey commented 1 month ago

Update: I've changed this PR so that the polyfill simply sets globalThis.__env__ to empty object. I'll wait to proceed until we've hashed out the discussion in https://github.com/unjs/unenv/pull/186.