chore: split readConfig and readPages config to simplify and lower nu…

…mber of responsibilities (#7472) cjhore: remove spurious code
cloudflare · Dec 6, 2024 · 119819f · 119819f
1 parent 3d8a652
commit 119819f
Show file tree

Hide file tree

Showing 7 changed files with 98 additions and 113 deletions.
diff --git a/packages/wrangler/src/api/pages/deploy.ts b/packages/wrangler/src/api/pages/deploy.ts
@@ -5,7 +5,7 @@ import path, { join, resolve as resolvePath } from "node:path";
 import { cwd } from "node:process";
 import { File, FormData } from "undici";
 import { fetchResult } from "../../cfetch";
-import { readConfig } from "../../config";
+import { readPagesConfig } from "../../config";
 import { shouldCheckFetch } from "../../deployment-bundle/bundle";
 import { validateNodeCompatMode } from "../../deployment-bundle/node-compat";
 import { FatalError } from "../../errors";
@@ -160,11 +160,7 @@ export async function deploy({
 	let config: Config | undefined;
 
 	try {
-		config = readConfig(
-			undefined,
-			{ ...args, env },
-			{ requirePagesConfig: true }
-		);
+		config = readPagesConfig(undefined, { ...args, env });
 	} catch (err) {
 		if (
 			!(

diff --git a/packages/wrangler/src/config/index.ts b/packages/wrangler/src/config/index.ts
@@ -73,65 +73,63 @@ type ReadConfigCommandArgs = NormalizeAndValidateConfigArgs;
  */
 export function readConfig(
 	configPath: string | undefined,
-	// Include command specific args as well as the wrangler global flags
 	args: ReadConfigCommandArgs,
-	options: { requirePagesConfig: true }
-): Omit<Config, "pages_build_output_dir"> & { pages_build_output_dir: string };
-export function readConfig(
-	configPath: string | undefined,
-	// Include command specific args as well as the wrangler global flags
-	args: ReadConfigCommandArgs,
-	options?: { requirePagesConfig?: boolean; hideWarnings?: boolean }
+	options?: { hideWarnings?: boolean }
 ): Config;
 export function readConfig(
 	configPath: string | undefined,
-	// Include command specific args as well as the wrangler global flags
 	args: ReadConfigCommandArgs,
-	{
-		requirePagesConfig,
-		hideWarnings = false,
-	}: { requirePagesConfig?: boolean; hideWarnings?: boolean } = {}
+	{ hideWarnings = false }: { hideWarnings?: boolean } = {}
 ): Config {
-	let rawConfig: RawConfig = {};
+	if (!configPath) {
+		configPath = findWranglerConfig(process.cwd());
+	}
+
+	const rawConfig = readRawConfig(configPath);
+
+	const { config, diagnostics } = normalizeAndValidateConfig(
+		rawConfig,
+		configPath,
+		args
+	);
+
+	if (diagnostics.hasWarnings() && !hideWarnings) {
+		logger.warn(diagnostics.renderWarnings());
+	}
+	if (diagnostics.hasErrors()) {
+		throw new UserError(diagnostics.renderErrors());
+	}
+
+	return config;
+}
 
+export function readPagesConfig(
+	configPath: string | undefined,
+	args: ReadConfigCommandArgs,
+	{ hideWarnings = false }: { hideWarnings?: boolean } = {}
+): Omit<Config, "pages_build_output_dir"> & { pages_build_output_dir: string } {
 	if (!configPath) {
 		configPath = findWranglerConfig(process.cwd());
 	}
 
+	let rawConfig: RawConfig;
 	try {
 		rawConfig = readRawConfig(configPath);
 	} catch (e) {
-		// Swallow parsing errors if we require a pages config file.
-		// At this point, we can't tell if the user intended to provide a Pages config file (and so should see the parsing error) or not (and so shouldn't).
-		// We err on the side of swallowing the error so as to not break existing projects
-		if (requirePagesConfig) {
-			logger.error(e);
-			throw new FatalError(
-				`Your ${configFileName(configPath)} file is not a valid Pages config file`,
-				EXIT_CODE_INVALID_PAGES_CONFIG
-			);
-		} else {
-			throw e;
-		}
+		logger.error(e);
+		throw new FatalError(
+			`Your ${configFileName(configPath)} file is not a valid Pages config file`,
+			EXIT_CODE_INVALID_PAGES_CONFIG
+		);
 	}
 
-	/**
-	 * Check if configuration file belongs to a Pages project.
-	 *
-	 * The `pages_build_output_dir` config key is used to determine if the
-	 * configuration file belongs to a Workers or Pages project. This key
-	 * should always be set for Pages but never for Workers.
-	 */
-	const isPagesConfigFile = isPagesConfig(rawConfig);
-	if (!isPagesConfigFile && requirePagesConfig) {
+	if (!isPagesConfig(rawConfig)) {
 		throw new FatalError(
 			`Your ${configFileName(configPath)} file is not a valid Pages config file`,
 			EXIT_CODE_INVALID_PAGES_CONFIG
 		);
 	}
 
-	// Process the top-level configuration. This is common for both
-	// Workers and Pages
 	const { config, diagnostics } = normalizeAndValidateConfig(
 		rawConfig,
 		configPath,
@@ -145,28 +143,24 @@ export function readConfig(
 		throw new UserError(diagnostics.renderErrors());
 	}
 
-	// If we detected a Pages project, run config file validation against
-	// Pages specific validation rules
-	if (isPagesConfigFile) {
-		logger.debug(
-			`Configuration file belonging to ⚡️ Pages ⚡️ project detected.`
-		);
+	logger.debug(
+		`Configuration file belonging to ⚡️ Pages ⚡️ project detected.`
+	);
 
-		const envNames = rawConfig.env ? Object.keys(rawConfig.env) : [];
-		const projectName = rawConfig?.name;
-		const pagesDiagnostics = validatePagesConfig(config, envNames, projectName);
+	const envNames = rawConfig.env ? Object.keys(rawConfig.env) : [];
+	const projectName = rawConfig?.name;
+	const pagesDiagnostics = validatePagesConfig(config, envNames, projectName);
 
-		if (pagesDiagnostics.hasWarnings()) {
-			logger.warn(pagesDiagnostics.renderWarnings());
-		}
-		if (pagesDiagnostics.hasErrors()) {
-			throw new UserError(pagesDiagnostics.renderErrors());
-		}
+	if (pagesDiagnostics.hasWarnings()) {
+		logger.warn(pagesDiagnostics.renderWarnings());
+	}
+	if (pagesDiagnostics.hasErrors()) {
+		throw new UserError(pagesDiagnostics.renderErrors());
 	}
 
-	applyPythonConfig(config, args);
-
-	return config;
+	return config as Omit<Config, "pages_build_output_dir"> & {
+		pages_build_output_dir: string;
+	};
 }
 
 export const readRawConfig = (configPath: string | undefined): RawConfig => {
@@ -178,26 +172,6 @@ export const readRawConfig = (configPath: string | undefined): RawConfig => {
 	}
 	return {};
 };
-/**
- * Modifies the provided config to support python workers, if the entrypoint is a .py file
- */
-function applyPythonConfig(config: Config, args: ReadConfigCommandArgs) {
-	const mainModule = "script" in args ? args.script : config.main;
-	if (typeof mainModule === "string" && mainModule.endsWith(".py")) {
-		// Workers with a python entrypoint should have bundling turned off, since all of Wrangler's bundling is JS/TS specific
-		config.no_bundle = true;
-
-		// Workers with a python entrypoint need module rules for "*.py". Add one automatically as a DX nicety
-		if (!config.rules.some((rule) => rule.type === "PythonModule")) {
-			config.rules.push({ type: "PythonModule", globs: ["**/*.py"] });
-		}
-		if (!config.compatibility_flags.includes("python_workers")) {
-			throw new UserError(
-				"The `python_workers` compatibility flag is required to use Python."
-			);
-		}
-	}
-}
 
 /**
  * Find the wrangler config file by searching up the file-system

diff --git a/packages/wrangler/src/config/validation.ts b/packages/wrangler/src/config/validation.ts
@@ -2,6 +2,7 @@ import assert from "node:assert";
 import path from "node:path";
 import TOML from "@iarna/toml";
 import { dedent } from "ts-dedent";
+import { UserError } from "../errors";
 import { getFlag } from "../experimental-flags";
 import { Diagnostics } from "./diagnostics";
 import {
@@ -309,9 +310,35 @@ export function normalizeAndValidateConfig(
 		[...Object.keys(config), "env", "$schema"]
 	);
 
+	applyPythonConfig(config, args);
+
 	return { config, diagnostics };
 }
 
+/**
+ * Modifies the provided config to support python workers, if the entrypoint is a .py file
+ */
+function applyPythonConfig(
+	config: Config,
+	args: NormalizeAndValidateConfigArgs
+) {
+	const mainModule = "script" in args ? args.script : config.main;
+	if (typeof mainModule === "string" && mainModule.endsWith(".py")) {
+		// Workers with a python entrypoint should have bundling turned off, since all of Wrangler's bundling is JS/TS specific
+		config.no_bundle = true;
+
+		// Workers with a python entrypoint need module rules for "*.py". Add one automatically as a DX nicety
+		if (!config.rules.some((rule) => rule.type === "PythonModule")) {
+			config.rules.push({ type: "PythonModule", globs: ["**/*.py"] });
+		}
+		if (!config.compatibility_flags.includes("python_workers")) {
+			throw new UserError(
+				"The `python_workers` compatibility flag is required to use Python."
+			);
+		}
+	}
+}
+
 /**
  * Validate the `build` configuration and return the normalized values.
  */

diff --git a/packages/wrangler/src/pages/build-env.ts b/packages/wrangler/src/pages/build-env.ts
@@ -1,6 +1,6 @@
 import { existsSync, writeFileSync } from "node:fs";
 import path from "node:path";
-import { configFileName, findWranglerConfig, readConfig } from "../config";
+import { configFileName, findWranglerConfig, readPagesConfig } from "../config";
 import { FatalError } from "../errors";
 import { logger } from "../logger";
 import {
@@ -56,15 +56,11 @@ export const Handler = async (args: PagesBuildEnvArgs) => {
 		pages_build_output_dir: string;
 	};
 	try {
-		config = readConfig(
-			configPath,
-			{
-				...args,
-				// eslint-disable-next-line turbo/no-undeclared-env-vars
-				env: process.env.PAGES_ENVIRONMENT,
-			},
-			{ requirePagesConfig: true }
-		);
+		config = readPagesConfig(configPath, {
+			...args,
+			// eslint-disable-next-line turbo/no-undeclared-env-vars
+			env: process.env.PAGES_ENVIRONMENT,
+		});
 	} catch (err) {
 		// found `wrangler.toml` but `pages_build_output_dir` is not specified
 		if (

diff --git a/packages/wrangler/src/pages/build.ts b/packages/wrangler/src/pages/build.ts
@@ -8,7 +8,7 @@ import path, {
 	resolve as resolvePath,
 } from "node:path";
 import { createUploadWorkerBundleContents } from "../api/pages/create-worker-bundle-contents";
-import { findWranglerConfig, readConfig } from "../config";
+import { findWranglerConfig, readPagesConfig } from "../config";
 import { shouldCheckFetch } from "../deployment-bundle/bundle";
 import { writeAdditionalModules } from "../deployment-bundle/find-additional-modules";
 import { validateNodeCompatMode } from "../deployment-bundle/node-compat";
@@ -359,15 +359,11 @@ async function maybeReadPagesConfig(
 		return undefined;
 	}
 	try {
-		const config = readConfig(
-			configPath,
-			{
-				...args,
-				// eslint-disable-next-line turbo/no-undeclared-env-vars
-				env: process.env.PAGES_ENVIRONMENT,
-			},
-			{ requirePagesConfig: true }
-		);
+		const config = readPagesConfig(configPath, {
+			...args,
+			// eslint-disable-next-line turbo/no-undeclared-env-vars
+			env: process.env.PAGES_ENVIRONMENT,
+		});
 
 		return {
 			...config,

diff --git a/packages/wrangler/src/pages/deploy.ts b/packages/wrangler/src/pages/deploy.ts
@@ -1,7 +1,7 @@
 import { execSync } from "node:child_process";
 import { deploy } from "../api/pages/deploy";
 import { fetchResult } from "../cfetch";
-import { configFileName, findWranglerConfig, readConfig } from "../config";
+import { configFileName, findWranglerConfig, readPagesConfig } from "../config";
 import { getConfigCache, saveToConfigCache } from "../config-cache";
 import { prompt, select } from "../dialogs";
 import { FatalError } from "../errors";
@@ -123,11 +123,7 @@ export const Handler = async (args: PagesDeployArgs) => {
 		 * need for now. We will perform a second config file read later
 		 * in `/api/pages/deploy`, that will get the environment specific config
 		 */
-		config = readConfig(
-			configPath,
-			{ ...args, env: undefined },
-			{ requirePagesConfig: true }
-		);
+		config = readPagesConfig(configPath, { ...args, env: undefined });
 	} catch (err) {
 		if (
 			!(

diff --git a/packages/wrangler/src/pages/secret/index.ts b/packages/wrangler/src/pages/secret/index.ts
@@ -2,7 +2,11 @@ import path from "node:path";
 import readline from "node:readline";
 import chalk from "chalk";
 import { fetchResult } from "../../cfetch";
-import { configFileName, findWranglerConfig, readConfig } from "../../config";
+import {
+	configFileName,
+	findWranglerConfig,
+	readPagesConfig,
+} from "../../config";
 import { getConfigCache } from "../../config-cache";
 import { confirm, prompt } from "../../dialogs";
 import { FatalError } from "../../errors";
@@ -49,11 +53,7 @@ async function pagesProject(
 		 * return the top-level config. This contains all the information we
 		 * need.
 		 */
-		config = readConfig(
-			configPath,
-			{ env: undefined },
-			{ requirePagesConfig: true }
-		);
+		config = readPagesConfig(configPath, { env: undefined });
 	} catch (err) {
 		if (
 			!(