Skip to content

feat: remove async_hooks polyfill, use native workerd implementation#10243

Merged
petebacondarwin merged 3 commits intomainfrom
devin/1754476737-remove-async-hooks-polyfill
Aug 7, 2025
Merged

feat: remove async_hooks polyfill, use native workerd implementation#10243
petebacondarwin merged 3 commits intomainfrom
devin/1754476737-remove-async-hooks-polyfill

Conversation

@devin-ai-integration
Copy link
Contributor

Fixes #10239

This PR removes the async_hooks polyfill from the unenv-preset package and switches to using the native workerd implementation. The change is based on the fact that workerd now provides native support for async_hooks APIs, making the polyfill unnecessary.

Changes Made

  • Moved async_hooks from hybridModules to nativeModules in packages/unenv-preset/src/preset.ts
  • Removed the polyfill implementation by deleting packages/unenv-preset/src/runtime/node/async_hooks.ts
  • Added comprehensive tests for async_hooks functionality in the e2e test suite
  • Updated module compatibility tests to include async_hooks in the native modules list

Key Findings

The native workerd implementation provides:

  • Full functionality for AsyncLocalStorage (the most commonly used API)
  • Working constructor for AsyncResource
  • Stub implementations for other APIs like createHook, executionAsyncId, etc. for backward compatibility
  • Limited AsyncResource methods - asyncId() and triggerAsyncId() are not implemented

Testing

Added two comprehensive test functions:

  • testAsyncHooks() - Tests AsyncLocalStorage, AsyncResource constructor, and stub APIs
  • testAsyncHooksRequire() - Tests CommonJS require pattern compatibility

All e2e tests pass (180 tests) confirming that the native implementation works correctly.

Review Focus Areas

⚠️ Critical for review:

  1. Compatibility impact - Verify that removing the polyfill won't break existing users
  2. AsyncResource limitations - Confirm that the missing asyncId()/triggerAsyncId() methods won't cause issues
  3. Version bump appropriateness - Major bump chosen for unenv-preset due to polyfill removal

Link to Devin run: https://app.devin.ai/sessions/d91bc6320b194eaa8db8a6715a8e9db5
Requested by: @petebacondarwin


  • Tests
    • Tests included
  • Public documentation
    • Documentation not necessary because: Internal implementation change with no user-facing API changes
  • Wrangler V3 Backport
    • Not necessary because: This is a feature change that enables native workerd functionality, not a bug fix

@devin-ai-integration
Copy link
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@changeset-bot
Copy link

changeset-bot bot commented Aug 6, 2025

🦋 Changeset detected

Latest commit: 2b91389

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@cloudflare/unenv-preset Minor
wrangler Patch
@cloudflare/vite-plugin Patch
@cloudflare/vitest-pool-workers Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@pkg-pr-new
Copy link

pkg-pr-new bot commented Aug 6, 2025

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@10243

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@10243

miniflare

npm i https://pkg.pr.new/miniflare@10243

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@10243

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@10243

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@10243

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@10243

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@10243

wrangler

npm i https://pkg.pr.new/wrangler@10243

commit: 2b91389

@devin-ai-integration devin-ai-integration bot marked this pull request as ready for review August 6, 2025 12:40
@devin-ai-integration devin-ai-integration bot requested a review from a team August 6, 2025 12:40
@devin-ai-integration devin-ai-integration bot requested a review from a team as a code owner August 6, 2025 12:40
@github-project-automation github-project-automation bot moved this from Untriaged to Approved in workers-sdk Aug 6, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Aug 6, 2025

Failed to automatically backport this PR's changes to Wrangler v3. Please manually create a PR targeting the v3-maintenance branch with your changes. Thank you for helping us keep Wrangler v3 supported!

Depending on your changes, running git rebase --onto v3-maintenance main devin/1754476737-remove-async-hooks-polyfill might be a good starting point.

Notes:

  • your PR branch should be named v3-backport-10243
  • add the skip-v3-pr label to the current PR to stop this workflow from failing

devin-ai-integration bot and others added 2 commits August 7, 2025 10:44
- Move async_hooks from hybridModules to nativeModules in unenv-preset
- Remove async_hooks polyfill implementation file
- Add comprehensive tests for AsyncLocalStorage and AsyncResource
- Verify native workerd async_hooks APIs work correctly

Fixes #10239

Co-Authored-By: pbacondarwin@cloudflare.com <pete@bacondarwin.com>
- Remove implementation details from changeset
- Keep description brief and targeted at tool users
- Address PR feedback from @petebacondarwin

Co-Authored-By: pbacondarwin@cloudflare.com <pete@bacondarwin.com>
@petebacondarwin petebacondarwin force-pushed the devin/1754476737-remove-async-hooks-polyfill branch from 6f4e350 to 0a8d741 Compare August 7, 2025 09:44
"_tls_wrap",
"assert",
"assert/strict",
"async_hooks",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does workerd implement the full async_hooks package? The documentation states:

Workers does not implement the full async_hooks ↗ API upon which Node.js' implementation of AsyncLocalStorage is built.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It implements all the methods that we were polyfilling.

@petebacondarwin petebacondarwin merged commit d481901 into main Aug 7, 2025
48 of 53 checks passed
@petebacondarwin petebacondarwin deleted the devin/1754476737-remove-async-hooks-polyfill branch August 7, 2025 15:25
@github-project-automation github-project-automation bot moved this from Approved to Done in workers-sdk Aug 7, 2025
@workers-devprod workers-devprod added the contribution [Holopin] Recognizes an open-source contribution, big or small label Aug 7, 2025
@holopin-bot
Copy link

holopin-bot bot commented Aug 7, 2025

Congratulations @devin-ai-integration[bot], the maintainer of this repository has issued you a holobyte! Here it is: https://holopin.io/holobyte/cme1jtzwt335607l7xnv2psof

This badge can only be claimed by you, so make sure that your GitHub account is linked to your Holopin account. You can manage those preferences here: https://holopin.io/account.
Or if you're new to Holopin, you can simply sign up with GitHub, which will do the trick!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contribution [Holopin] Recognizes an open-source contribution, big or small

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

async_hooks doesn't need to be a hybrid module

4 participants