Fix nodejs compat v2 upgrade #6824

petebacondarwin · 2024-09-25T14:07:12Z

What this PR solves / how to test

We recently added a compatibility date to allow the nodejs_compat flag to imply nodejs_compat_v2.
Wrangler and Miniflare needed to be updated to apply the appropriate build-time aspects for this flag + date combination.

Author has addressed the following

Tests
- TODO (before merge)
- Tests included
- Tests not necessary because:
E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
- I don't know
- Required
- Not required because:
Changeset (Changeset guidelines)
- TODO (before merge)
- Changeset included
- Changeset not necessary because:
Public documentation
- TODO (before merge)
- Cloudflare docs PR(s):
- Documentation not necessary because: bug fixes

changeset-bot · 2024-09-25T14:07:17Z

🦋 Changeset detected

Latest commit: 7db433a

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

This PR includes changesets to release 4 packages

Name	Type
wrangler	Patch
miniflare	Patch
@cloudflare/vitest-pool-workers	Patch
@cloudflare/pages-shared	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 · 2024-09-25T14:11:01Z

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/11039417113/npm-package-wrangler-6824

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

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

Or you can use npx with this latest build directly:

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

Additional artifacts:

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

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

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

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

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

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

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

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.20240909.5
`workerd`	1.20240925.0	1.20240925.0
`workerd --version`	1.20240925.0	2024-09-25

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

packages/wrangler/src/__tests__/deploy.test.ts

packages/miniflare/src/plugins/core/node-compat.ts

packages/wrangler/src/deployment-bundle/build-failures.ts

packages/miniflare/src/plugins/core/modules.ts

packages/miniflare/src/plugins/core/node-compat.ts

vicb · 2024-09-25T18:53:39Z

packages/miniflare/src/plugins/core/node-compat.ts

+ *   and also some Node.js globals such as `Buffer`; it also turns on additional compile-time polyfills for those that are not provided by the runtime.
+ *  - null - no Node.js compatibility.
+ */
+export type NodeJSCompatMode = "legacy" | "als" | "v1" | "v2" | null;


Just a thought, not asking any change here.

I think we add to confusion with those names.

It is already confusing to have nodejs_compat_v2 equivalent to nodejs_compat && compatibility_date >= "2024-09-23"

But naming this mode "v2" is adding unnecessary confusion. I think a descriptive name could have been better than "legacy", "v1, "v2" (and maybe "none" instead of "null").

No blaming anyone here - there are probably historic reasons. But something to keep in mind for the future.

Actually I disagree here. I tried out various different descriptive names but in order to describe the two versions adequately they became way to verbose and in any case didn't really make much difference.

We have always talked about the two versions of runtime nodejs_compat

v1 only handles importing built-ins prefixed with node: and doesn't add the globals

v2 handles non-prefixed imports, adds globals, and further Wrangler bundles in additional polyfills.

Moreover the temporary flag to turn v2 on was indeed called nodejs_compat_v2 not a more descriptive name.

Fixes #6822

A compatibility of Sept 23, 2024 or later means that `nodejs_compat` is equivalent to `nodejs_compat_v2`.

This logic is now also needed inside Miniflare to determine how to handle node imports. So this commit moves the relevant code in there and references it from Wrangler. The validation of the config is kept in Wrangler, since Miniflare doesn't need that particularly and the validation relies upon Wrangler logger and UserError.

A compatibility of Sept 23, 2024 or later means that `nodejs_compat` is equivalent to `nodejs_compat_v2`.

Now that we can set the compat date to Sept 23, 2024, we can update this fixture not to use the `nodejs_compat_v2` flag.

…flare

petebacondarwin requested review from a team as code owners September 25, 2024 14:07

petebacondarwin added the e2e Run e2e tests on a PR label Sep 25, 2024