search

unjs / unenv

🕊️ Convert javaScript code to be runtime agnostic
MIT License
465 stars 22 forks source link

feat(cloudflare): implement `node:module` as an hybrid polyfill #301

Closed vicb closed 2 months ago

vicb commented 2 months ago

Related workerd PR: https://github.com/cloudflare/workerd/pull/2636

This PR changes module to be an hybrid polyfill and use the workerd available method implementations.

Is there anything else to change/update?

Thanks!

/cc @pi0

pi0 commented 2 months ago

Nice start dear @vicb.

Next step, we need to create src/runtime/node/module/$cloudflare.ts that reexports index.ts except overriding createRequire to pick from process.getBuiltinModule('module'). (See this as an example).

I am putting PR in WIP state until changes on both side are finalized.

vicb commented 2 months ago

Nice start dear @vicb.

Thanks Pooya ;)

I have tried to do something. The idea is to be able to merge the PR even before the workerd implementation is updated.

I probably got a few things wrong, but I'll learn...

Thanks for the review

vicb commented 2 months ago

Is the last version better?

vicb commented 2 months ago

Thanks ❤️

LGTM only we should check process.getBuiltinModule esbuild issue with @IgorMinar

I think using process.getBuiltinModule is ok, it is unpatched:

https://github.com/unjs/unenv/blob/70db6f157dde54d1b11f4fa77cee630fb255be49/src/runtime/node/process/%24cloudflare.ts#L210-L215

Confirmed that it WAI on a simple wrangler pjt