unjs / unenv

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

feat(cloudflare): introduce hybrid polyfills for the cloudflare preset #224

Closed IgorMinar closed 1 month ago

IgorMinar commented 1 month ago

This preset provides hybrid polyfills (https://github.com/unjs/unenv/issues/181) but requires special workerd flags.

Due to potential for breaking changes and extra requirements, we created a new preset rather than updating the existing one.

In the next major version of unenv, we should remove the cloudflare preset and rename cloudflareV2 to cloudflare.

๐Ÿ”— Linked issue

https://github.com/unjs/unenv/issues/181

โ“ Type of change

๐Ÿ“š Description

๐Ÿ“ Checklist

IgorMinar commented 1 month ago

@pi0 this one is a bit controversial as it introduces a new preset.

We could modify the existing cloudflare preset, but that would most likely be a breaking change for existing nitro apps, so we need to be careful here.

I'm really hoping that this extra preset is just a temporary thing we need to do to avoid a breaking change, but once nitro/nuxt is updated, we should drop the old preset and keep just this new one.

I called the preset cloudflareV2 but we could call it cloudflareExperimental or similar, because using that preset does currently require extra handholding.

What do you think?

pi0 commented 1 month ago

Thanks for the PR!

Currently unenv's cloudflare preset is not used by any version of Nuxt and Nitro i mainly made it few months ago to initiate an experiment but we hit the wrangler.toml automation and canceled so i think we can safely merge them. (Plus, i think considering risk of current changes, next version of unenv would be v2 with an explicit upgrade in current Nuxt, Nitro and H3 releases) ๐Ÿ‘๐Ÿผ

pi0 commented 1 month ago

@IgorMinar Just to double check is process.getBuiltinModule GA available or expected soon to be available?

IgorMinar commented 1 month ago

@IgorMinar Just to double check is process.getBuiltinModule GA available or expected soon to be available?

it will be. we'll need to coordinate the GA with unenv@2.0.0

IgorMinar commented 1 month ago

Thanks for the PR!

Currently unenv's cloudflare preset is not used by any version of Nuxt and Nitro i mainly made it few months ago to initiate an experiment but we hit the wrangler.toml automation and canceled so i think we can safely merge them. (Plus, i think considering risk of current changes, next version of unenv would be v2 with an explicit upgrade in current Nuxt, Nitro and H3 releases) ๐Ÿ‘๐Ÿผ

Excellent! I wasn't sure about the existing usage of the preset. I also agree that v2 will make getting this out easier.

In that case, I will go ahead and merge the cloudflare and cloudflareV2 presents into one.

IgorMinar commented 1 month ago

@pi0 I addressed all your review feedback. PTAL

IgorMinar commented 1 month ago

@jculvey those will be in separate PRs. I don't want to expand the scope of this one any more. I just wanted to implement a few to prove that the technique works for many modules.

pi0 commented 1 month ago

Feel free to merge whenever ready @IgorMinar i'm fine with treeshaking being deferred.

pi0 commented 1 month ago

In #242 i'm moving node internals to /internal dirs. Not sure if makes sense but we could also use /cloudflare namespaces if it makes sense (current $ seems also nice..)

IgorMinar commented 1 month ago

Feel free to merge whenever ready @IgorMinar i'm fine with treeshaking being deferred.

@pi0 I just finished the refactoring and pushed. It turned out to be a slightly bigger refactor but nothing too surprising: https://github.com/unjs/unenv/pull/224/commits/f23952445a8e2919f9e728d4a1c89c3f1c75a9f1

If CI is also green, I'll merge.