Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Feb 19, 2025

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

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

changeset-bot bot commented Feb 19, 2025

🦋 Changeset detected

Latest commit: 9addcea

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

Copy link
Contributor

github-actions bot commented Feb 19, 2025

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/13443167933/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/13443167933/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/13443167933/npm-package-cloudflare-workers-bindings-extension-8191 -O ./cloudflare-workers-bindings-extension.0.0.0-v070d28207.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v070d28207.vsix

create-cloudflare:

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

@cloudflare/kv-asset-handler:

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

miniflare:

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

@cloudflare/pages-shared:

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

@cloudflare/unenv-preset:

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

@cloudflare/vite-plugin:

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

@cloudflare/vitest-pool-workers:

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

@cloudflare/workers-editor-shared:

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

@cloudflare/workers-shared:

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

@cloudflare/workflows-shared:

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

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


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

@vicb vicb added the e2e Run wrangler e2e tests on a PR label Feb 19, 2025
@vicb vicb requested a review from petebacondarwin February 19, 2025 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e Run wrangler e2e tests on a PR
Projects
Status: Untriaged
Development

Successfully merging this pull request may close these issues.

1 participant