BLOCKED fix: remove `experimental:` prefix requirement for nodejs_compat_v2 #6545

petebacondarwin · 2024-08-21T13:07:28Z

See https://jira.cfdata.org/browse/DEVDASH-218

What this PR solves / how to test

Fixes #[insert GH or internal issue number(s)].

Author has addressed the following

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

changeset-bot · 2024-08-21T13:07:32Z

🦋 Changeset detected

Latest commit: 8cb9690

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 · 2024-08-21T13:10:23Z

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/10508136058/npm-package-wrangler-6545

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

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

Or you can use npx with this latest build directly:

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

Additional artifacts:

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

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

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

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

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

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

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

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

wrangler@3.72.1 includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20240806.1
`workerd`	1.20240821.1	1.20240821.1
`workerd --version`	1.20240821.1	2024-08-21

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

.changeset/green-lies-crash.md

vicb · 2024-08-21T13:45:55Z

packages/wrangler/src/deployment-bundle/node-compat.ts

- * Currently we require that the v2 mode is configured via `experimental:nodejs_compat_v2` compat flag,
- * where the `experimental:` prefix is stripped before being passed to the runtime since that does not
- * understand this prefix.
+ * Currently we require that the v2 mode is configured via `nodejs_compat_v2` compat flag.


"Currently" -> "As of September 2024" ?

It is actually whenever we land this and release Wrangler.
So I'd rather not put a date in here.

vicb · 2024-08-21T13:48:38Z

packages/wrangler/src/deployment-bundle/node-compat.ts

-	const nodejsCompatV2 = compatibility_flags.includes(
-		"experimental:nodejs_compat_v2"
-	);
+	const nodejsCompatV2 = compatibility_flags.includes("nodejs_compat_v2");


nodejsCompatV2 === true implies nodejsCompat === true because of the common prefix - this is probably a bug?

A few more nits - not directly related to this PR.

What about renaming node_compat to nodeCompatLegacy?

Could setting legacy use mode instead of node_compat?

Any reason we are not using enums for flag names and mode?

nodejsCompatV2 === true implies nodejsCompat === true because of the common prefix - this is probably a bug?

compatibility_flags is an array of strings, so these include() calls are correct.
E.g.

["nodejs_compat_v2"].includes("nodejs_compat") === false

What about renaming node_compat to nodeCompatLegacy?

It is called node_compat in the config object, which is passed here.

Any reason we are not using enums for flag names and mode?

We don't tend to use enums much in this code base. It is nice to have strings when debugging.
Any reason to use enums?

Could setting legacy use mode instead of node_compat?

Not sure what you mean here?

To be clear, what this function does is compute the mode, but also return flags telling us if any of these properties were turned on, so we can check the validaty of combinations.

Any reason to use enums?

Avoid repeating a string with the risk of a typo.
enum

can be strings enum Mode { V1 = "v1", Legacy="legacy"}

support autocomplete

support exhaustive switch (i.e. warn/error when adding a value and not updating a switch)

are easier to search in the code (arguably)

Not sure what you mean here?

legacy: node_compat === true, -> legacy: mode === "legacy",

Just to avoid repeating logic as repeating might lead to out of sync code

Any reason to use enums?

Avoid repeating a string with the risk of a typo. enum

can be strings enum Mode { V1 = "v1", Legacy="legacy"}

support autocomplete

support exhaustive switch (i.e. warn/error when adding a value and not updating a switch)

are easier to search in the code (arguably)

Since the type is a narrow collection of const strings we get all those already: https://www.typescriptlang.org/play/?#code/KYDwDg9gTgLgBDAnmYcByEAmwBSBlAYQgFswBDGAWS1QF44AiAG2AHMyBjRBuAH0YBuARh78GAgEyi4AOwCuTJgG4AUCo4QZAZ3hkAXOhr4ipCtWxx6zNp26qN2+ACMDGbMZLkqNS4JH3NHTgOVyNCTzMfK0kGAMc4TFD3cNNvC3p5RTigpzJEw2STL3M6RgAzCAgGNWwOJjIoVDK5GQ4YAEtNOFZgNOAACgBKJNwU4ppVFS0Ad3aYDgALOH6evqHBuABvFThd4LItVGt2LgY9Hb3Lp0ayAGtVS44Do+Ezi8vd6+A7h72nw8EUnOHz2Xx+732AIYuUwbxBnxu9wh-1QmSYwPhYKRAF8gA

Not sure what you mean here?

legacy: node_compat === true, -> legacy: mode === "legacy",

Just to avoid repeating logic as repeating might lead to out of sync code

Ah I see! I can create an extra var to DRY that.

IgorMinar

lgtm!

one more thing to consider: do we want to issue a nice warning to users that are currently using the experimental prefix? could we error and tell them that the flag is no longer experimental and they should update their config rather than error with message saying that the flag doesn't exist, which I think will happen once this change is merged.

IgorMinar · 2024-08-21T15:12:14Z

packages/wrangler/src/deployment-bundle/node-compat.ts

@@ -94,39 +78,25 @@ export function getNodeCompatMode({
 	compatibility_flags,
 	node_compat,
 }: Pick<Config, "compatibility_flags" | "node_compat">) {
+	const legacy = node_compat === true;


isn't v1 also considered a legacy mode?

what will this code look like when v2 is the default and v1 is legacy (which should happen soon). consider tweaking this name a bit to make it more future proof.

Legacy is a special pre-nodejs_compat mode.
After we got nodejs_compat we go into versioned mode, e.g. v1, v2, etc.
V1 is not legacy it is just an older version of the same mode.

Oh, it reminds me one nice thing with enum: you can see the JSDoc when hovering on a value

We will support it forever, but it will become flagged on the combination of compatibilty flag (nodejs_compat) and the compatibility date (e.g. older than X September 2024).

petebacondarwin · 2024-08-22T07:33:56Z

one more thing to consider: do we want to issue a nice warning to users that are currently using the experimental prefix? could we error and tell them that the flag is no longer experimental and they should update their config rather than error with message saying that the flag doesn't exist, which I think will happen once this change is merged.

Very good point. I'll add that.

See https://jira.cfdata.org/browse/DEVDASH-218

vicb · 2024-08-27T14:47:26Z

packages/wrangler/src/deployment-bundle/node-compat.ts

 		mode = "legacy";
 	} else {
 		mode = null;
 	}

 	return {
-		legacy: node_compat === true,
+		legacy,


I would love this function to return a single value for clarity.

It looks to me that legacy, nodejsCompat, and nodejsCompatv2 can be collapsed into mode.

experimentalNodejsCompatV2 might be handled in a different functions.

RamIdeas · 2024-08-28T14:03:48Z

Closing in favour of #6593

petebacondarwin added the blocked Blocked on other work label Aug 21, 2024

petebacondarwin mentioned this pull request Aug 21, 2024

Nodejs compat v2 on pages #6511

Merged

12 tasks

vicb requested changes Aug 21, 2024

View reviewed changes

IgorMinar approved these changes Aug 21, 2024

View reviewed changes

Base automatically changed from nodejs-compat-v2-on-pages to main August 21, 2024 17:34

petebacondarwin force-pushed the remove-nodejs_v2_experimental branch from 569679a to 7b8d4ea Compare August 22, 2024 07:25

petebacondarwin mentioned this pull request Aug 22, 2024

fix: ignore nodejs_compat flags in runtime type generation #6554

Merged

12 tasks

petebacondarwin added 5 commits August 22, 2024 13:32

fix: remove experimental: prefix requirement for nodejs_compat_v2

bb49618

See https://jira.cfdata.org/browse/DEVDASH-218

PR review updates 2

16598c6

PR review udpdates

1cbc70e

add error message for invalid use of experimental prefix

64a8048

reworking parameters after feedback

8cb9690

petebacondarwin force-pushed the remove-nodejs_v2_experimental branch from 7b8d4ea to 8cb9690 Compare August 22, 2024 12:32

vicb reviewed Aug 27, 2024

View reviewed changes

vicb mentioned this pull request Aug 28, 2024

fix: remove experimental: prefix requirement for nodejs_compat_v2 #6593

Merged

RamIdeas closed this Aug 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BLOCKED fix: remove `experimental:` prefix requirement for nodejs_compat_v2 #6545

BLOCKED fix: remove `experimental:` prefix requirement for nodejs_compat_v2 #6545

petebacondarwin commented Aug 21, 2024

changeset-bot bot commented Aug 21, 2024 •

edited

Loading

github-actions bot commented Aug 21, 2024 •

edited

Loading

vicb Aug 21, 2024

petebacondarwin Aug 21, 2024

vicb Aug 21, 2024

vicb Aug 21, 2024

petebacondarwin Aug 21, 2024

petebacondarwin Aug 21, 2024 •

edited

Loading

petebacondarwin Aug 21, 2024

petebacondarwin Aug 21, 2024

vicb Aug 21, 2024

vicb Aug 21, 2024

petebacondarwin Aug 21, 2024

petebacondarwin Aug 21, 2024

IgorMinar left a comment

IgorMinar Aug 21, 2024

petebacondarwin Aug 21, 2024

vicb Aug 21, 2024

petebacondarwin Aug 22, 2024

petebacondarwin commented Aug 22, 2024

vicb Aug 27, 2024

RamIdeas commented Aug 28, 2024

BLOCKED fix: remove experimental: prefix requirement for nodejs_compat_v2 #6545

BLOCKED fix: remove experimental: prefix requirement for nodejs_compat_v2 #6545

Conversation

petebacondarwin commented Aug 21, 2024

What this PR solves / how to test

Author has addressed the following

changeset-bot bot commented Aug 21, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorMinar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Aug 22, 2024

Choose a reason for hiding this comment

RamIdeas commented Aug 28, 2024

BLOCKED fix: remove `experimental:` prefix requirement for nodejs_compat_v2 #6545

BLOCKED fix: remove `experimental:` prefix requirement for nodejs_compat_v2 #6545

changeset-bot bot commented Aug 21, 2024 •

edited

Loading

github-actions bot commented Aug 21, 2024 •

edited

Loading

petebacondarwin Aug 21, 2024 •

edited

Loading