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

IgorMinar · 2024-05-30T19:29:22Z

This preset provides hybrid polyfills (#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

#181

❓ Type of change

📖 Documentation (updates to the documentation, readme, or JSdoc annotations)
🐞 Bug fix (a non-breaking change that fixes an issue)
👌 Enhancement (improving an existing functionality like performance)
✨ New feature (a non-breaking change that adds functionality)
🧹 Chore (updates to the build process or auxiliary tools and libraries)
⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

📝 Checklist

I have linked an issue or discussion.
I have updated the documentation accordingly.

IgorMinar · 2024-05-30T19:34:31Z

@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 · 2024-05-31T14:49:28Z

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 · 2024-05-31T14:50:12Z

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

pi0 · 2024-05-31T14:51:14Z

src/runtime/node/crypto/cloudflare.ts

I would suggest to adopt a convention like $cloudflare or $workerd for file naming to clearly indicate they are platform-specific overrides, really nice idea btw we could extend this convention later

I'm good with $cloudflare. I'll update the PR.

IgorMinar · 2024-05-31T15:34:49Z

@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 [email protected]

IgorMinar · 2024-05-31T15:37:28Z

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 · 2024-05-31T16:19:33Z

@pi0 I addressed all your review feedback. PTAL

pi0 · 2024-05-31T16:26:01Z

src/runtime/node/crypto/$cloudflare.ts

+  scryptSync,
+  sign,
+  verify,
+} = unenvCrypto;


Would it make sense that we use export { ... } from './index' to improve the tree-shaking possibility (in case of ESM imports)?

I believe that this will tree-shake just fine because everything is statically analyzable, but I'll verify.

I think bundler would be confused because we first import as namespace then use (runtime) restructure. At least it cannot be a safe treeshake for it because namespace import can have side-effects as well as destructure

there is indeed a problem with tree-shaking. take a look at this...

Destructured import usage:

current

fixed

Namespaced import usage:

current

fixed for real

So the last pattern is the way to go even though it's the most verbose. Oh well...

This seems like the kind of optimization that rollup could handle automatically. Maybe one day. I got too spoiled by the capabilities of Closure Compiler at Google.

src/runtime/node/crypto/$cloudflare.ts

pi0

Other than two suggestions seems nice upgrade!

jculvey

Changes LGTM.

Quick sanity check: it seems like there's quite a few more modules that need similar treatment: diagnostics_check, async_hooks, path, process, stream...etc

Are we going to tackle those in a followup PR?

IgorMinar · 2024-05-31T18:53:57Z

@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 · 2024-05-31T19:50:17Z

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

pi0 · 2024-05-31T19:51:59Z

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..)

This preset now provides hybrid polyfills which temporarily require special workerd compat flags. This is most likely a breaking change, and will require us to release it as [email protected]

See unjs#224 (comment)

IgorMinar · 2024-05-31T20:23:11Z

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: f239524

If CI is also green, I'll merge.

IgorMinar requested a review from pi0 as a code owner May 30, 2024 19:29

IgorMinar requested a review from jculvey May 30, 2024 19:30

IgorMinar force-pushed the feat/workerd-hybrid-buffer branch 3 times, most recently from 0046dba to 3e58051 Compare May 31, 2024 07:29

pi0 reviewed May 31, 2024

View reviewed changes

IgorMinar force-pushed the feat/workerd-hybrid-buffer branch from 3e58051 to d338793 Compare May 31, 2024 16:16

IgorMinar changed the title ~~feat: add experimental cloudflareV2 preset~~ feat(cloudflare): introduce hybrid polyfills for the cloudflare preset May 31, 2024

IgorMinar force-pushed the feat/workerd-hybrid-buffer branch from d338793 to 800ebd8 Compare May 31, 2024 16:18

pi0 reviewed May 31, 2024

View reviewed changes

src/runtime/node/crypto/$cloudflare.ts Outdated Show resolved Hide resolved

pi0 approved these changes May 31, 2024

View reviewed changes

jculvey approved these changes May 31, 2024

View reviewed changes

IgorMinar force-pushed the feat/workerd-hybrid-buffer branch from 800ebd8 to 3a05249 Compare May 31, 2024 18:57

IgorMinar added 7 commits May 31, 2024 13:19

feat(cloudflare): introduce hybrid polyfills into the cloudflare preset

02b7621

This preset now provides hybrid polyfills which temporarily require special workerd compat flags. This is most likely a breaking change, and will require us to release it as [email protected]

feat(cloudflare): recreate node:util as a hybrid polyfill

3241587

feat(cloudflare): implement node:utils/types as a hybrid polyfill

79a80f2

feat(cloudflare): implement node:crypto as a hybrid polyfill

7c75bad

fixup: implementing review feedback

eb0abf4

fixup: update the node:sys alias after rebase

5c1ed89

fixup: more review feedback - simplifying the default exports

1050a3b

fixup: make the polyfills tree-shakable

f239524

See unjs#224 (comment)

IgorMinar force-pushed the feat/workerd-hybrid-buffer branch from dda197a to f239524 Compare May 31, 2024 20:22

IgorMinar merged commit 3931166 into unjs:main May 31, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

IgorMinar commented May 30, 2024

IgorMinar commented May 30, 2024

pi0 commented May 31, 2024

pi0 commented May 31, 2024

pi0 May 31, 2024

IgorMinar May 31, 2024

IgorMinar commented May 31, 2024

IgorMinar commented May 31, 2024

IgorMinar commented May 31, 2024 •

edited

Loading

pi0 May 31, 2024

IgorMinar May 31, 2024

pi0 May 31, 2024 •

edited

Loading

IgorMinar May 31, 2024 •

edited

Loading

IgorMinar May 31, 2024

pi0 left a comment

jculvey left a comment

IgorMinar commented May 31, 2024

pi0 commented May 31, 2024

pi0 commented May 31, 2024

IgorMinar commented May 31, 2024

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

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

Conversation

IgorMinar commented May 30, 2024

🔗 Linked issue

❓ Type of change

📚 Description

📝 Checklist

IgorMinar commented May 30, 2024

pi0 commented May 31, 2024

pi0 commented May 31, 2024

pi0 May 31, 2024

Choose a reason for hiding this comment

IgorMinar May 31, 2024

Choose a reason for hiding this comment

IgorMinar commented May 31, 2024

IgorMinar commented May 31, 2024

IgorMinar commented May 31, 2024 • edited Loading

pi0 May 31, 2024

Choose a reason for hiding this comment

IgorMinar May 31, 2024

Choose a reason for hiding this comment

pi0 May 31, 2024 • edited Loading

Choose a reason for hiding this comment

IgorMinar May 31, 2024 • edited Loading

Choose a reason for hiding this comment

IgorMinar May 31, 2024

Choose a reason for hiding this comment

pi0 left a comment

Choose a reason for hiding this comment

jculvey left a comment

Choose a reason for hiding this comment

IgorMinar commented May 31, 2024

pi0 commented May 31, 2024

pi0 commented May 31, 2024

IgorMinar commented May 31, 2024

IgorMinar commented May 31, 2024 •

edited

Loading

pi0 May 31, 2024 •

edited

Loading

IgorMinar May 31, 2024 •

edited

Loading