fix(hybrid-nodejs-compat): inject modules only once by vicb · Pull Request #8191 · cloudflare/workers-sdk

vicb · 2025-02-19T12:51:17Z

This PR makes sure that modules are injected only once before injected all the related values.

You can see the effect for (from the Cloudflare preset):

  inject: {
    setImmediate: ["unenv/runtime/node/timers/$cloudflare", "setImmediate"],
    clearImmediate: ["unenv/runtime/node/timers/$cloudflare", "clearImmediate"]
  },

The generated code before this PR:

// ../../packages/wrangler/_virtual_unenv_global_polyfill-set$immediate.js
var init_virtual_unenv_global_polyfill_set_immediate = __esm({
  "../../packages/wrangler/_virtual_unenv_global_polyfill-set$immediate.js"() {
    init_cloudflare();
    globalThis.setImmediate = setImmediateFallback;
  }
});

// ../../packages/wrangler/_virtual_unenv_global_polyfill-clear$immediate.js
var init_virtual_unenv_global_polyfill_clear_immediate = __esm({
  "../../packages/wrangler/_virtual_unenv_global_polyfill-clear$immediate.js"() {
    init_cloudflare();
    globalThis.clearImmediate = clearImmediateFallback;
  }
});

var init_utils = __esm({
  "../../node_modules/.pnpm/unenv@2.0.0-rc.1/node_modules/unenv/runtime/_internal/utils.mjs"() {
    init_virtual_unenv_global_polyfill_process();
    init_virtual_unenv_global_polyfill_performance();
    init_virtual_unenv_global_polyfill_console();
    // BEFORE {
    init_virtual_unenv_global_polyfill_set_immediate();
    init_virtual_unenv_global_polyfill_clear_immediate();
    // } BEFORE
    __name(mergeFns, "mergeFns");
    __name(createNotImplementedError, "createNotImplementedError");
    __name(notImplemented, "notImplemented");
    __name(notImplementedAsync, "notImplementedAsync");
    __name(notImplementedClass, "notImplementedClass");
  }
});

and after this PR:

// ../../packages/wrangler/_virtual_unenv_global_polyfill-unenv-runtime-node-timers-$cloudflare
var init_virtual_unenv_global_polyfill_unenv_runtime_node_timers_cloudflare = __esm({
  "../../packages/wrangler/_virtual_unenv_global_polyfill-unenv-runtime-node-timers-$cloudflare"() {
    init_cloudflare();
    globalThis.setImmediate = setImmediateFallback;
    globalThis.clearImmediate = clearImmediateFallback;
  }
});

var init_utils = __esm({
  "../../node_modules/.pnpm/unenv@2.0.0-rc.1/node_modules/unenv/runtime/_internal/utils.mjs"() {
    init_virtual_unenv_global_polyfill_unenv_runtime_node_process_cloudflare();
    init_virtual_unenv_global_polyfill_unenv_runtime_polyfill_performance();
    init_virtual_unenv_global_polyfill_unenv_runtime_node_console_cloudflare();
    // AFTER {
    init_virtual_unenv_global_polyfill_unenv_runtime_node_timers_cloudflare();
    // } AFTER
    __name(mergeFns, "mergeFns");
    __name(createNotImplementedError, "createNotImplementedError");
    __name(notImplemented, "notImplemented");
    __name(notImplementedAsync, "notImplementedAsync");
    __name(notImplementedClass, "notImplementedClass");
  }
});

So the first thing is that the code size is reduced - as a data point init_virtual_unenv_global_polyfill_unenv_runtime_node_console_cloudflare(); / init_virtual_unenv_global_polyfill_unenv_runtime_node_timers_cloudflare(); is repeated 106 times in the nodejs-hybrid-app test.

Another benefit (which actually is the reason for which I created this PR) is that you can inject multiple export from the same file when they have a dependency.

For examples, we would like to inject performance and Performance from perf_hooks. However performance needs to instantiate a Performance so there is no way we could initialize with the current code.

Let's say Performance is injected first from perf_hooks. So perf_hooks will be imported. The first thing that module will do (see init_utils above) would be to inject all the values. It will then try to inject performance which imports perf_hooks too which result in ESBuild doing nothing (because of the circular dependency) and then it can instantiate Performance because it is undefined.

One last benefit is that we would stop using the polyfills (i.e. init_virtual_unenv_global_polyfill_unenv_runtime_polyfill_performance();) as I think the preferred way is to use inject (the performance polyfill directly adds props on globalThis)

Tests
- TODO (before merge)
- Tests included
- Tests not necessary because: covered by the existing tests (I also ran the OpenNext tests)
Wrangler E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
- I don't know
- Required
- Not required because:
Public documentation
- TODO (before merge)
- Cloudflare docs PR(s):
- Documentation not necessary because: no user facing change

changeset-bot · 2025-02-19T12:51:21Z

🦋 Changeset detected

Latest commit: 99fd8cf

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

This PR includes changesets to release 2 packages

Name	Type
wrangler	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

github-actions · 2025-02-19T13:04:26Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13519060360/npm-package-wrangler-8191

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/8191/npm-package-wrangler-8191

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13519060360/npm-package-wrangler-8191 dev path/to/script.js

Additional artifacts:

cloudflare-workers-bindings-extension:

wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13519060360/npm-package-cloudflare-workers-bindings-extension-8191 -O ./cloudflare-workers-bindings-extension.0.0.0-v528e94d6c.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v528e94d6c.vsix

create-cloudflare:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13519060360/npm-package-create-cloudflare-8191 --no-auto-update

@cloudflare/kv-asset-handler:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13519060360/npm-package-cloudflare-kv-asset-handler-8191

miniflare:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13519060360/npm-package-miniflare-8191

@cloudflare/pages-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13519060360/npm-package-cloudflare-pages-shared-8191

@cloudflare/unenv-preset:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13519060360/npm-package-cloudflare-unenv-preset-8191

@cloudflare/vite-plugin:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13519060360/npm-package-cloudflare-vite-plugin-8191

@cloudflare/vitest-pool-workers:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13519060360/npm-package-cloudflare-vitest-pool-workers-8191

@cloudflare/workers-editor-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13519060360/npm-package-cloudflare-workers-editor-shared-8191

@cloudflare/workers-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13519060360/npm-package-cloudflare-workers-shared-8191

@cloudflare/workflows-shared:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13519060360/npm-package-cloudflare-workflows-shared-8191

Note that these links will no longer work once the GitHub Actions artifact expires.

wrangler@3.109.3 includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20250214.0
`workerd`	1.20250214.0	1.20250214.0
`workerd --version`	1.20250214.0	2025-02-14

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

petebacondarwin

LGTM with a few nits that you can take a call on.

.changeset/beige-buses-poke.md

packages/wrangler/src/__tests__/hybrid-nodejs-compat.test.ts

packages/wrangler/src/deployment-bundle/esbuild-plugins/hybrid-nodejs-compat.ts

petebacondarwin · 2025-02-25T09:58:50Z

I wonder if we could build these helpers into our preset package somehow?
Then we could reuse it in the Vite plugin, etc.

Co-authored-by: Pete Bacon Darwin <pete@bacondarwin.com>

vicb · 2025-02-25T10:31:30Z

I wonder if we could build these helpers into our preset package somehow? Then we could reuse it in the Vite plugin, etc.

Let's add polyfill (and other potential updates for unenv) and then we can think about sharing this code but it would be great.

vicb · 2025-02-25T10:32:20Z

Thanks for the review @petebacondarwin.
I have integrated your feedback - it makes the code much simpler!
PTAL

vicb requested a review from a team as a code owner February 19, 2025 12:51

vicb force-pushed the import-modules branch from a0f97c2 to 609ff5d Compare February 19, 2025 13:15

vicb added the e2e Run wrangler + vite-plugin e2e tests on a PR label Feb 19, 2025

vicb requested a review from petebacondarwin February 19, 2025 13:15

vicb force-pushed the import-modules branch 2 times, most recently from 3d2c3f7 to 2589173 Compare February 24, 2025 14:45

petebacondarwin approved these changes Feb 25, 2025

View reviewed changes

vicb and others added 3 commits February 25, 2025 11:12

fix(hybrid-nodejs-compat): inject modules only once

66a3ebd

Update .changeset/beige-buses-poke.md

980c991

Co-authored-by: Pete Bacon Darwin <pete@bacondarwin.com>

fixup! integrate review feedback

99fd8cf

vicb force-pushed the import-modules branch from 992c287 to 99fd8cf Compare February 25, 2025 10:28

vicb merged commit 968c3d9 into main Feb 25, 2025
18 checks passed

vicb deleted the import-modules branch February 25, 2025 12:57

workers-devprod mentioned this pull request Feb 25, 2025

Version Packages #8242

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(hybrid-nodejs-compat): inject modules only once#8191

fix(hybrid-nodejs-compat): inject modules only once#8191
vicb merged 3 commits intomainfrom
import-modules

vicb commented Feb 19, 2025 •

edited

Loading

Uh oh!

changeset-bot bot commented Feb 19, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Feb 19, 2025 •

edited

Loading

Uh oh!

petebacondarwin left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petebacondarwin commented Feb 25, 2025

Uh oh!

vicb commented Feb 25, 2025

Uh oh!

vicb commented Feb 25, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

vicb commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petebacondarwin commented Feb 25, 2025

Uh oh!

vicb commented Feb 25, 2025

Uh oh!

vicb commented Feb 25, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vicb commented Feb 19, 2025 •

edited

Loading

changeset-bot bot commented Feb 19, 2025 •

edited

Loading

github-actions bot commented Feb 19, 2025 •

edited

Loading