unjs / unenv

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

refactor(node/buffer): update buffer to be cloudflare compat module #334

Closed anonrig closed 1 month ago

anonrig commented 1 month ago

Latest version of workerd adds all missing functionality of node:buffer and exposes node:sys and sys modules.

FYI: This requires workerd v1.20241011.0, which is included in the main branch of unenv, but not in the main branch of wrangler.

vicb commented 1 month ago

FYI: This requires the latest version of workerd, which is included in the main branch of unenv, but not in the main branch of wrangler.

Could you specify the exact version and mark this as draft until wrangler is updated?

anonrig commented 1 month ago

Could you specify the exact version and mark this as draft until wrangler is updated?

These changes are included in https://github.com/cloudflare/workerd/releases/tag/v1.20241011.0.

anonrig commented 1 month ago

cc @unjs/cloudflare

anonrig commented 1 month ago

@pi0 Everytime we change the test file, it seems your review is required in order to land it. Can you review and land this pull-request please?

vicb commented 1 month ago

@pi0 Everytime we change the test file, it seems your review is required in order to land it. Can you review and land this pull-request please?

See https://github.com/unjs/unenv/pull/326#issuecomment-2408149169

pi0 commented 1 month ago

Everytime we change the test file, it seems your review is required in order to land it. Can you review and land this pull-request please?

Dear @anonrig i have already explained it in https://github.com/unjs/unenv/pull/326#issuecomment-2408149169 Workerd tests added in #311 are both for wrangler/node_compat and unenv (polyfill) isolate tests itself. unenv is a shared project. No one did a review on tests PR, at least you could reach out to me asking it's purpose before trying to push changes on it and saying it is a blocker.

Sadly today only because of supporting wrangler SDK, unenv got completely broken for it's main users (Nuxt and Nitro). i hear no single response from anyone of cloudflare team about it for weeks yet i am at airport, offday, evening, doing prompt reviewers to help not making situation worst than this.

Sorry for telling all these but i am trying to do all my best to unblock your team while wrangler, blocked unenv project to be used for it's primary user (Nuxt and Nitro), i hope it can help you understand situation.

southpolesteve commented 1 month ago

@pi0 👋 One of the Workers EMs here. First off, I know you're doing a ton of work here to help unblock us. Its much appreciated. Fully acknowledge this is a shared project with many users not just wrangler.

Sorry for the lack of communication on our side. I think we've got couple of wires crossed internally and I'll work with the team to resolve that.

You mentioned in the other thread "proper channels". Can you add me to wherever that is? I am happy to jump in to make sure talking more and doing everything we can do support the shared needs of the project. sfaulkner@cloudflare.com is my email

pi0 commented 1 month ago

Hi, dear Steve @southpolesteve I appreciate your help on this matter.

We mainly used discord channels to communicate and i mentioned our blockers many times both public and private. I am more than happy to find any better way to improve our communication, we all love cloudflare! If you happen to be on discord, i believe someone from the team can send you an invitation (or please direct me @pi0).

In the meantime, let's unblock this situation first.

pi0 commented 1 month ago

Temporarily merged #326 until finding a better solution.

@anonrig if you can kindly help to maintain workerd tests coverage for nodeless that would be best otherwise please feel free to merge it as hotfix and iterate.

anonrig commented 1 month ago

I've updated the tests to have 2 different paths. @pi0

vicb commented 1 month ago

Discussed with @anonrig offline: I have reverted the changes for sys and will merge this PR. Thanks all!