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

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/green-lies-crash.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"wrangler": patch
---

fix: remove `experimental:` prefix requirement for nodejs_compat_v2

See https://jira.cfdata.org/browse/DEVDASH-218
2 changes: 1 addition & 1 deletion fixtures/nodejs-hybrid-app/wrangler.toml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name = "nodejs-hybrid-app"
main = "src/index.ts"
compatibility_date = "2024-06-03"
compatibility_flags = ["experimental:nodejs_compat_v2"]
compatibility_flags = ["nodejs_compat_v2"]

[vars]
# These DB connection values are to a public database containing information about
Expand Down
2 changes: 1 addition & 1 deletion fixtures/pages-nodejs-v2-compat/wrangler.toml
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
name = "pages-nodejs-compat"
compatibility_date = "2024-08-20"
compatibility_flags = ["experimental:nodejs_compat_v2"]
compatibility_flags = ["nodejs_compat_v2"]
8 changes: 4 additions & 4 deletions packages/wrangler/src/__tests__/deploy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9845,10 +9845,10 @@ export default{
"deploy index.js --no-bundle --node-compat --dry-run --outdir dist"
);
expect(std.warn).toMatchInlineSnapshot(`
"▲ [WARNING] Enabling Wrangler compile-time Node.js compatibility polyfill mode for builtins and globals. This is experimental and has serious tradeoffs.
"▲ [WARNING] \`--node-compat\` and \`--no-bundle\` can't be used together. If you want to polyfill Node.js built-ins and disable Wrangler's bundling, please polyfill as part of your own bundling process.


▲ [WARNING] \`--node-compat\` and \`--no-bundle\` can't be used together. If you want to polyfill Node.js built-ins and disable Wrangler's bundling, please polyfill as part of your own bundling process.
▲ [WARNING] Enabling Wrangler compile-time Node.js compatibility polyfill mode for builtins and globals. This is experimental and has serious tradeoffs.

"
`);
Expand All @@ -9865,10 +9865,10 @@ export default{
fs.writeFileSync("index.js", scriptContent);
await runWrangler("deploy index.js --dry-run --outdir dist");
expect(std.warn).toMatchInlineSnapshot(`
"▲ [WARNING] Enabling Wrangler compile-time Node.js compatibility polyfill mode for builtins and globals. This is experimental and has serious tradeoffs.
"▲ [WARNING] \`--node-compat\` and \`--no-bundle\` can't be used together. If you want to polyfill Node.js built-ins and disable Wrangler's bundling, please polyfill as part of your own bundling process.


▲ [WARNING] \`--node-compat\` and \`--no-bundle\` can't be used together. If you want to polyfill Node.js built-ins and disable Wrangler's bundling, please polyfill as part of your own bundling process.
▲ [WARNING] Enabling Wrangler compile-time Node.js compatibility polyfill mode for builtins and globals. This is experimental and has serious tradeoffs.

"
`);
Expand Down
11 changes: 5 additions & 6 deletions packages/wrangler/src/api/pages/deploy.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -174,12 +174,11 @@ export async function deploy({
}
}

const nodejsCompatMode = validateNodeCompat({
legacyNodeCompat: false,
compatibilityFlags:
config?.compatibility_flags ?? deploymentConfig.compatibility_flags ?? [],
noBundle: config?.no_bundle ?? false,
});
const nodejsCompatMode = validateNodeCompat(
config?.compatibility_flags ?? deploymentConfig.compatibility_flags ?? [],
/* node_compat */ false,
config?.no_bundle
);
const defineNavigatorUserAgent = isNavigatorDefined(
config?.compatibility_date ?? deploymentConfig.compatibility_date,
config?.compatibility_flags ?? deploymentConfig.compatibility_flags
Expand Down
8 changes: 4 additions & 4 deletions packages/wrangler/src/deploy/deploy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -397,11 +397,11 @@ See https://developers.cloudflare.com/workers/platform/compatibility-dates for m

const compatibilityFlags =
props.compatibilityFlags ?? config.compatibility_flags;
const nodejsCompatMode = validateNodeCompat({
legacyNodeCompat: props.nodeCompat ?? config.node_compat ?? false,
const nodejsCompatMode = validateNodeCompat(
compatibilityFlags,
noBundle: props.noBundle ?? config.no_bundle ?? false,
});
props.nodeCompat ?? config.node_compat,
props.noBundle ?? config.no_bundle
);

// Warn if user tries minify with no-bundle
if (props.noBundle && minify) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { readFileSync } from "node:fs";
import path from "node:path";
import { File, FormData } from "undici";
import { handleUnsafeCapnp } from "./capnp";
import { stripExperimentalPrefixes } from "./node-compat";
import type {
CfDurableObjectMigrations,
CfModuleType,
Expand Down Expand Up @@ -547,7 +546,7 @@ export function createWorkerUploadForm(worker: CfWorkerInit): FormData {
bindings: metadataBindings,
...(compatibility_date && { compatibility_date }),
...(compatibility_flags && {
compatibility_flags: stripExperimentalPrefixes(compatibility_flags),
compatibility_flags,
}),
...(migrations && { migrations }),
capnp_schema: capnpSchemaOutputFile,
Expand Down
96 changes: 37 additions & 59 deletions packages/wrangler/src/deployment-bundle/node-compat.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { UserError } from "../errors";
import { logger } from "../logger";
import type { Config } from "../config";

/**
* Wrangler can provide Node.js compatibility in a number of different modes:
Expand All @@ -17,85 +16,76 @@ export type NodeJSCompatMode = "legacy" | "v1" | "v2" | null;
* Returns one of:
* - "legacy": build-time polyfills, from `node_compat` flag
* - "v1": nodejs_compat compatibility flag
* - "v2": experimental nodejs_compat_v2 flag
* - "v2": nodejs_compat_v2 compatibility flag
* - null: no Node.js compatibility
*
* 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 v2 mode is configured via `nodejs_compat_v2` compat flag.
* At a future compatibility date, the use of `nodejs_compat` flag will imply `nodejs_compat_v2`.
*
* We assert that only one of these modes can be specified at a time.
* We assert that you must prefix v2 mode with `experimental`.
* We warn if using legacy or v2 mode.
*/
export function validateNodeCompat({
legacyNodeCompat,
compatibilityFlags,
noBundle,
}: {
legacyNodeCompat: boolean;
/* mutate */ compatibilityFlags: string[];
noBundle: boolean;
}): NodeJSCompatMode {
if (legacyNodeCompat) {
logger.warn(
"Enabling Wrangler compile-time Node.js compatibility polyfill mode for builtins and globals. This is experimental and has serious tradeoffs."
export function validateNodeCompat(
compatibilityFlags: string[],
nodeCompat: boolean | undefined,
noBundle: boolean | undefined
): NodeJSCompatMode {
const {
mode,
nodejsCompat,
nodejsCompatV2,
experimentalNodejsCompatV2,
legacy,
} = getNodeCompatMode(compatibilityFlags, nodeCompat);

if (experimentalNodejsCompatV2) {
throw new UserError(
"The `experimental:` prefix on `nodejs_compat_v2` is no longer valid. Please remove it and try again."
);
}

const { mode, nodejsCompat, nodejsCompatV2 } = getNodeCompatMode({
compatibility_flags: compatibilityFlags,
node_compat: legacyNodeCompat,
});

const nodejsCompatV2NotExperimental =
compatibilityFlags.includes("nodejs_compat_v2");

if (nodejsCompat && nodejsCompatV2) {
throw new UserError(
"The `nodejs_compat` and `nodejs_compat_v2` compatibility flags cannot be used in together. Please select just one."
);
}

if (legacyNodeCompat && (nodejsCompat || nodejsCompatV2)) {
if (legacy && (nodejsCompat || nodejsCompatV2)) {
throw new UserError(
`The ${nodejsCompat ? "`nodejs_compat`" : "`nodejs_compat_v2`"} compatibility flag cannot be used in conjunction with the legacy \`--node-compat\` flag. If you want to use the Workers ${nodejsCompat ? "`nodejs_compat`" : "`nodejs_compat_v2`"} compatibility flag, please remove the \`--node-compat\` argument from your CLI command or \`node_compat = true\` from your config file.`
);
}

if (nodejsCompatV2NotExperimental) {
throw new UserError(
`The \`nodejs_compat_v2\` compatibility flag is experimental and must be prefixed with \`experimental:\`. Use \`experimental:nodejs_compat_v2\` flag instead.`
);
}

if (nodejsCompatV2) {
if (noBundle && legacy) {
logger.warn(
"Enabling experimental Node.js compatibility mode v2. This feature is still in development and not ready for production use."
"`--node-compat` and `--no-bundle` can't be used together. If you want to polyfill Node.js built-ins and disable Wrangler's bundling, please polyfill as part of your own bundling process."
);
}

if (noBundle && legacyNodeCompat) {
if (noBundle && nodejsCompatV2) {
logger.warn(
"`--node-compat` and `--no-bundle` can't be used together. If you want to polyfill Node.js built-ins and disable Wrangler's bundling, please polyfill as part of your own bundling process."
"`nodejs_compat_v2` compatibility flag and `--no-bundle` can't be used together. If you want to polyfill Node.js built-ins and disable Wrangler's bundling, please polyfill as part of your own bundling process."
);
}

if (noBundle && nodejsCompatV2) {
if (mode === "legacy") {
logger.warn(
"`nodejs_compat_v2` compatibility flag and `--no-bundle` can't be used together. If you want to polyfill Node.js built-ins and disable Wrangler's bundling, please polyfill as part of your own bundling process."
"Enabling Wrangler compile-time Node.js compatibility polyfill mode for builtins and globals. This is experimental and has serious tradeoffs."
);
}

return mode;
}

export function getNodeCompatMode({
compatibility_flags,
node_compat,
}: Pick<Config, "compatibility_flags" | "node_compat">) {
const nodejsCompat = compatibility_flags.includes("nodejs_compat");
const nodejsCompatV2 = compatibility_flags.includes(
export function getNodeCompatMode(
compatibilityFlags: string[],
nodeCompat: boolean | undefined
) {
const legacy = nodeCompat === true;
const nodejsCompat = compatibilityFlags.includes("nodejs_compat");
const nodejsCompatV2 = compatibilityFlags.includes("nodejs_compat_v2");
const experimentalNodejsCompatV2 = compatibilityFlags.includes(
"experimental:nodejs_compat_v2"
);

Expand All @@ -104,29 +94,17 @@ export function getNodeCompatMode({
mode = "v2";
} else if (nodejsCompat) {
mode = "v1";
} else if (node_compat) {
} else if (legacy) {
mode = "legacy";
} else {
mode = null;
}

return {
legacy: node_compat === true,
legacy,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

mode,
nodejsCompat,
nodejsCompatV2,
experimentalNodejsCompatV2,
};
}

/**
* The nodejs_compat_v2 flag currently requires an `experimental:` prefix within Wrangler,
* but this needs to be stripped before sending to workerd, since that doesn't know about that.
*
* TODO: Remove this function when we graduate nodejs_v2 to non-experimental.
* See https://jira.cfdata.org/browse/DEVDASH-218
*/
export function stripExperimentalPrefixes(
compatFlags: string[] | undefined
): string[] | undefined {
return compatFlags?.map((flag) => flag.replace(/^experimental:/, ""));
}
35 changes: 15 additions & 20 deletions packages/wrangler/src/dev.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -686,15 +686,11 @@ export async function startDev(args: StartDevOptions) {
moduleRoot: args.moduleRoot,
moduleRules: args.rules,
nodejsCompatMode: (parsedConfig: Config) =>
validateNodeCompat({
legacyNodeCompat:
args.nodeCompat ?? parsedConfig.node_compat ?? false,
compatibilityFlags:
args.compatibilityFlags ??
parsedConfig.compatibility_flags ??
[],
noBundle: args.noBundle ?? parsedConfig.no_bundle ?? false,
}),
validateNodeCompat(
args.compatibilityFlags ?? parsedConfig.compatibility_flags ?? [],
args.nodeCompat ?? parsedConfig.node_compat,
args.noBundle ?? parsedConfig.no_bundle
),
},
bindings: {
...(await getPagesAssetsFetcher(
Expand Down Expand Up @@ -839,12 +835,11 @@ export async function startDev(args: StartDevOptions) {
additionalModules,
} = await validateDevServerSettings(args, config);

const nodejsCompatMode = validateNodeCompat({
legacyNodeCompat: args.nodeCompat ?? config.node_compat ?? false,
compatibilityFlags:
args.compatibilityFlags ?? config.compatibility_flags ?? [],
noBundle: args.noBundle ?? config.no_bundle ?? false,
});
const nodejsCompatMode = validateNodeCompat(
args.compatibilityFlags ?? config.compatibility_flags ?? [],
args.nodeCompat ?? config.node_compat,
args.noBundle ?? config.no_bundle
);

void metrics.sendMetricsEvent(
"run dev",
Expand Down Expand Up @@ -979,11 +974,11 @@ export async function startApiDev(args: StartDevOptions) {
additionalModules,
} = await validateDevServerSettings(args, config);

const nodejsCompatMode = validateNodeCompat({
legacyNodeCompat: args.nodeCompat ?? config.node_compat ?? false,
compatibilityFlags: args.compatibilityFlags ?? config.compatibility_flags,
noBundle: args.noBundle ?? config.no_bundle ?? false,
});
const nodejsCompatMode = validateNodeCompat(
args.compatibilityFlags ?? config.compatibility_flags,
args.nodeCompat ?? config.node_compat,
args.noBundle ?? config.no_bundle
);

await metrics.sendMetricsEvent(
"run dev (api)",
Expand Down
5 changes: 1 addition & 4 deletions packages/wrangler/src/dev/miniflare.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import {
} from "../ai/fetcher";
import { readConfig } from "../config";
import { ModuleTypeToRuleType } from "../deployment-bundle/module-collection";
import { stripExperimentalPrefixes } from "../deployment-bundle/node-compat";
import { withSourceURLs } from "../deployment-bundle/source-url";
import { UserError } from "../errors";
import { logger } from "../logger";
Expand Down Expand Up @@ -892,9 +891,7 @@ export async function buildMiniflareOptions(
{
name: getName(config),
compatibilityDate: config.compatibilityDate,
compatibilityFlags: stripExperimentalPrefixes(
config.compatibilityFlags
),
compatibilityFlags: config.compatibilityFlags,

...sourceOptions,
...bindingOptions,
Expand Down
3 changes: 1 addition & 2 deletions packages/wrangler/src/dev/remote.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import { useErrorHandler } from "react-error-boundary";
import { helpIfErrorIsSizeOrScriptStartup } from "../deploy/deploy";
import { printBundleSize } from "../deployment-bundle/bundle-reporter";
import { getBundleType } from "../deployment-bundle/bundle-type";
import { stripExperimentalPrefixes } from "../deployment-bundle/node-compat";
import { withSourceURLs } from "../deployment-bundle/source-url";
import { getInferredHost } from "../dev";
import { UserError } from "../errors";
Expand Down Expand Up @@ -669,7 +668,7 @@ export async function createRemoteWorkerInit(props: {
},
migrations: undefined, // no migrations in dev
compatibility_date: props.compatibilityDate,
compatibility_flags: stripExperimentalPrefixes(props.compatibilityFlags),
compatibility_flags: props.compatibilityFlags,
keepVars: true,
keepSecrets: true,
logpush: false,
Expand Down
12 changes: 6 additions & 6 deletions packages/wrangler/src/pages/build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -437,12 +437,12 @@ const validateArgs = async (args: PagesBuildArgs): Promise<ValidatedArgs> => {
args.outfile = resolvePath(args.outfile);
}

const { nodeCompat: legacyNodeCompat, ...argsExceptNodeCompat } = args;
const nodejsCompatMode = validateNodeCompat({
legacyNodeCompat: legacyNodeCompat,
compatibilityFlags: args.compatibilityFlags ?? [],
noBundle: config?.no_bundle ?? false,
});
const { nodeCompat: node_compat, ...argsExceptNodeCompat } = args;
const nodejsCompatMode = validateNodeCompat(
args.compatibilityFlags ?? [],
node_compat,
config?.no_bundle
);

const defineNavigatorUserAgent = isNavigatorDefined(
args.compatibilityDate,
Expand Down
11 changes: 5 additions & 6 deletions packages/wrangler/src/pages/dev.ts
Original file line number Diff line number Diff line change
Expand Up @@ -360,12 +360,11 @@ export const Handler = async (args: PagesDevArguments) => {

let scriptPath = "";

const nodejsCompatMode = validateNodeCompat({
legacyNodeCompat: args.nodeCompat,
compatibilityFlags:
args.compatibilityFlags ?? config.compatibility_flags ?? [],
noBundle: args.noBundle ?? config.no_bundle ?? false,
});
const nodejsCompatMode = validateNodeCompat(
args.compatibilityFlags ?? config.compatibility_flags ?? [],
args.nodeCompat,
args.noBundle ?? config.no_bundle
);

const defineNavigatorUserAgent = isNavigatorDefined(
compatibilityDate,
Expand Down
5 changes: 4 additions & 1 deletion packages/wrangler/src/type-generation/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,10 @@ export async function typesHandler(
const tsconfigPath =
config.tsconfig ?? join(dirname(configPath), "tsconfig.json");
const tsconfigTypes = readTsconfigTypes(tsconfigPath);
const { mode } = getNodeCompatMode(config);
const { mode } = getNodeCompatMode(
config.compatibility_flags,
config.node_compat
);

logRuntimeTypesMessage(outFile, tsconfigTypes, mode !== null);
}
Expand Down
Loading
Loading