From 02a0e1e186706eaec46048252068713f04698384 Mon Sep 17 00:00:00 2001
From: Taylor <1628134+taylorlee@users.noreply.github.com>
Date: Tue, 26 Nov 2024 04:54:32 -0800
Subject: [PATCH] Separate config for preview_urls (#7227)

* feat(wrangler): create separate config for workers_dev_previews

The current Preview URLs (beta) feature routes to version preview urls based on the status of the `workers_dev` config value. Beta users have requested the ability to enable deployment urls and preview urls separately on `workers.dev`, and the new `previews_enabled` field of the enable-subdomain API will allow that. This change separates the `workers_dev` and `workers_dev_previews` behavior during `wrangler triggers deploy` and `wrangler versions upload`. `preview_urls` defaults to true, and does not implicitly depend on routes the way `workers_dev` does.

* Rename to preview_urls

* Update .changeset/famous-mayflies-knock.md

Co-authored-by: emily-shen <69125074+emily-shen@users.noreply.github.com>

---------

Co-authored-by: Daniel Walsh <walshy@cloudflare.com>
Co-authored-by: Daniel Walsh <walshydev@gmail.com>
Co-authored-by: emily-shen <69125074+emily-shen@users.noreply.github.com>
---
 .changeset/famous-mayflies-knock.md           |   7 +
 .../src/__tests__/configuration.test.ts       |   1 +
 .../wrangler/src/__tests__/deploy.test.ts     |  94 ++++++++++-
 .../helpers/mock-workers-subdomain.ts         |  14 +-
 .../versions/versions.upload.test.ts          |   6 +-
 packages/wrangler/src/config/config.ts        |   1 +
 packages/wrangler/src/config/environment.ts   |   9 +
 packages/wrangler/src/config/validation.ts    |  10 ++
 packages/wrangler/src/init.ts                 |   2 +
 packages/wrangler/src/triggers/deploy.ts      | 156 ++++++++++--------
 packages/wrangler/src/versions/upload.ts      |   9 +-
 11 files changed, 224 insertions(+), 85 deletions(-)
 create mode 100644 .changeset/famous-mayflies-knock.md

diff --git a/.changeset/famous-mayflies-knock.md b/.changeset/famous-mayflies-knock.md
new file mode 100644
index 000000000000..98e7210b31f3
--- /dev/null
+++ b/.changeset/famous-mayflies-knock.md
@@ -0,0 +1,7 @@
+---
+"wrangler": minor
+---
+
+Add `preview_urls` toggle to `wrangler.toml`
+
+The current Preview URLs (beta) feature routes to version preview urls based on the status of the `workers_dev` config value. Beta users have requested the ability to enable deployment urls and preview urls separately on `workers.dev`, and the new `previews_enabled` field of the enable-subdomain API will allow that. This change separates the `workers_dev` and `preview_urls` behavior during `wrangler triggers deploy` and `wrangler versions upload`. `preview_urls` defaults to true, and does not implicitly depend on routes the way `workers_dev` does.
diff --git a/packages/wrangler/src/__tests__/configuration.test.ts b/packages/wrangler/src/__tests__/configuration.test.ts
index 6979789b260c..f785bf2a0637 100644
--- a/packages/wrangler/src/__tests__/configuration.test.ts
+++ b/packages/wrangler/src/__tests__/configuration.test.ts
@@ -120,6 +120,7 @@ describe("normalizeAndValidateConfig()", () => {
 			wasm_modules: undefined,
 			data_blobs: undefined,
 			workers_dev: undefined,
+			preview_urls: true,
 			zone_id: undefined,
 			no_bundle: undefined,
 			minify: undefined,
diff --git a/packages/wrangler/src/__tests__/deploy.test.ts b/packages/wrangler/src/__tests__/deploy.test.ts
index a59768f8d4ba..c99ddd44e334 100644
--- a/packages/wrangler/src/__tests__/deploy.test.ts
+++ b/packages/wrangler/src/__tests__/deploy.test.ts
@@ -5135,7 +5135,7 @@ addEventListener('fetch', event => {});`
 			expect(std.err).toMatchInlineSnapshot(`""`);
 		});
 
-		it("should not try to enable the workers.dev domain if it has been enabled before", async () => {
+		it("should not try to enable the workers.dev domain if it has been enabled before and previews are in sync", async () => {
 			writeWranglerConfig({
 				workers_dev: true,
 			});
@@ -5157,6 +5157,53 @@ addEventListener('fetch', event => {});`
 			expect(std.err).toMatchInlineSnapshot(`""`);
 		});
 
+		it("should sync the workers.dev domain if it has been enabled before but previews should be enabled", async () => {
+			writeWranglerConfig({
+				workers_dev: true,
+			});
+			writeWorkerSource();
+			mockUploadWorkerRequest();
+			mockGetWorkerSubdomain({ enabled: true, previews_enabled: false });
+			mockSubDomainRequest();
+			mockUpdateWorkerSubdomain({ enabled: true, previews_enabled: true });
+
+			await runWrangler("deploy ./index");
+
+			expect(std.out).toMatchInlineSnapshot(`
+				"Total Upload: xx KiB / gzip: xx KiB
+				Worker Startup Time: 100 ms
+				Uploaded test-name (TIMINGS)
+				Deployed test-name triggers (TIMINGS)
+				  https://test-name.test-sub-domain.workers.dev
+				Current Version ID: Galaxy-Class"
+			`);
+			expect(std.err).toMatchInlineSnapshot(`""`);
+		});
+
+		it("should sync the workers.dev domain if it has been enabled before but previews should be enabled", async () => {
+			writeWranglerConfig({
+				workers_dev: true,
+				preview_urls: false,
+			});
+			writeWorkerSource();
+			mockUploadWorkerRequest();
+			mockGetWorkerSubdomain({ enabled: true, previews_enabled: true });
+			mockSubDomainRequest();
+			mockUpdateWorkerSubdomain({ enabled: true, previews_enabled: false });
+
+			await runWrangler("deploy ./index");
+
+			expect(std.out).toMatchInlineSnapshot(`
+				"Total Upload: xx KiB / gzip: xx KiB
+				Worker Startup Time: 100 ms
+				Uploaded test-name (TIMINGS)
+				Deployed test-name triggers (TIMINGS)
+				  https://test-name.test-sub-domain.workers.dev
+				Current Version ID: Galaxy-Class"
+			`);
+			expect(std.err).toMatchInlineSnapshot(`""`);
+		});
+
 		it("should disable the workers.dev domain if workers_dev is `false`", async () => {
 			writeWranglerConfig({
 				workers_dev: false,
@@ -5178,7 +5225,7 @@ addEventListener('fetch', event => {});`
 			expect(std.err).toMatchInlineSnapshot(`""`);
 		});
 
-		it("should not try to disable the workers.dev domain if it is not already available", async () => {
+		it("should not try to disable the workers.dev domain if it is not already available and previews are in sync", async () => {
 			writeWranglerConfig({
 				workers_dev: false,
 			});
@@ -5200,6 +5247,49 @@ addEventListener('fetch', event => {});`
 			expect(std.err).toMatchInlineSnapshot(`""`);
 		});
 
+		it("should sync the workers.dev domain if it is not available but previews should be enabled", async () => {
+			writeWranglerConfig({
+				workers_dev: false,
+			});
+			writeWorkerSource();
+			mockUploadWorkerRequest();
+			mockGetWorkerSubdomain({ enabled: false, previews_enabled: false });
+			mockUpdateWorkerSubdomain({ enabled: false, previews_enabled: true });
+
+			await runWrangler("deploy ./index");
+
+			expect(std.out).toMatchInlineSnapshot(`
+				"Total Upload: xx KiB / gzip: xx KiB
+				Worker Startup Time: 100 ms
+				Uploaded test-name (TIMINGS)
+				No deploy targets for test-name (TIMINGS)
+				Current Version ID: Galaxy-Class"
+			`);
+			expect(std.err).toMatchInlineSnapshot(`""`);
+		});
+
+		it("should sync the workers.dev domain if it is not available but previews should be disabled", async () => {
+			writeWranglerConfig({
+				workers_dev: false,
+				preview_urls: false,
+			});
+			writeWorkerSource();
+			mockUploadWorkerRequest();
+			mockGetWorkerSubdomain({ enabled: false, previews_enabled: true });
+			mockUpdateWorkerSubdomain({ enabled: false, previews_enabled: false });
+
+			await runWrangler("deploy ./index");
+
+			expect(std.out).toMatchInlineSnapshot(`
+				"Total Upload: xx KiB / gzip: xx KiB
+				Worker Startup Time: 100 ms
+				Uploaded test-name (TIMINGS)
+				No deploy targets for test-name (TIMINGS)
+				Current Version ID: Galaxy-Class"
+			`);
+			expect(std.err).toMatchInlineSnapshot(`""`);
+		});
+
 		it("should disable the workers.dev domain if workers_dev is undefined but overwritten to `false` in environment", async () => {
 			writeWranglerConfig({
 				env: {
diff --git a/packages/wrangler/src/__tests__/helpers/mock-workers-subdomain.ts b/packages/wrangler/src/__tests__/helpers/mock-workers-subdomain.ts
index c96c9f7e14cc..3185346ba6d8 100644
--- a/packages/wrangler/src/__tests__/helpers/mock-workers-subdomain.ts
+++ b/packages/wrangler/src/__tests__/helpers/mock-workers-subdomain.ts
@@ -36,11 +36,13 @@ export function mockSubDomainRequest(
 /** Create a mock handler to fetch the  <script>.<user>.workers.dev subdomain status*/
 export function mockGetWorkerSubdomain({
 	enabled,
+	previews_enabled = true,
 	env,
 	legacyEnv = false,
 	expectedScriptName = "test-name",
 }: {
 	enabled: boolean;
+	previews_enabled?: boolean;
 	env?: string | undefined;
 	legacyEnv?: boolean | undefined;
 	expectedScriptName?: string;
@@ -61,7 +63,9 @@ export function mockGetWorkerSubdomain({
 					expect(params.envName).toEqual(env);
 				}
 
-				return HttpResponse.json(createFetchResult({ enabled }));
+				return HttpResponse.json(
+					createFetchResult({ enabled, previews_enabled })
+				);
 			},
 			{ once: true }
 		)
@@ -71,11 +75,13 @@ export function mockGetWorkerSubdomain({
 /** Create a mock handler to toggle a <script>.<user>.workers.dev subdomain status */
 export function mockUpdateWorkerSubdomain({
 	enabled,
+	previews_enabled = true,
 	env,
 	legacyEnv = false,
 	expectedScriptName = "test-name",
 }: {
 	enabled: boolean;
+	previews_enabled?: boolean;
 	env?: string | undefined;
 	legacyEnv?: boolean | undefined;
 	expectedScriptName?: string;
@@ -96,8 +102,10 @@ export function mockUpdateWorkerSubdomain({
 					expect(params.envName).toEqual(env);
 				}
 				const body = await request.json();
-				expect(body).toEqual({ enabled });
-				return HttpResponse.json(createFetchResult({ enabled }));
+				expect(body).toEqual({ enabled, previews_enabled });
+				return HttpResponse.json(
+					createFetchResult({ enabled, previews_enabled })
+				);
 			},
 			{ once: true }
 		)
diff --git a/packages/wrangler/src/__tests__/versions/versions.upload.test.ts b/packages/wrangler/src/__tests__/versions/versions.upload.test.ts
index acd714e91288..8a2e46fd12c9 100644
--- a/packages/wrangler/src/__tests__/versions/versions.upload.test.ts
+++ b/packages/wrangler/src/__tests__/versions/versions.upload.test.ts
@@ -110,7 +110,7 @@ describe("versions upload", () => {
 	test("should print preview url if version has preview", async () => {
 		mockGetScript();
 		mockUploadVersion(true);
-		mockGetWorkerSubdomain({ enabled: true });
+		mockGetWorkerSubdomain({ enabled: true, previews_enabled: true });
 		mockSubDomainRequest();
 
 		// Setup
@@ -140,10 +140,10 @@ describe("versions upload", () => {
 		`);
 	});
 
-	it("should not print preview url workers_dev is false", async () => {
+	it("should not print preview url when preview_urls is false", async () => {
 		mockGetScript();
 		mockUploadVersion(true);
-		mockGetWorkerSubdomain({ enabled: false });
+		mockGetWorkerSubdomain({ enabled: true, previews_enabled: false });
 
 		// Setup
 		writeWranglerConfig({
diff --git a/packages/wrangler/src/config/config.ts b/packages/wrangler/src/config/config.ts
index 6cc0bfde2707..5b12cc58adf3 100644
--- a/packages/wrangler/src/config/config.ts
+++ b/packages/wrangler/src/config/config.ts
@@ -355,6 +355,7 @@ export const defaultWranglerConfig: Config = {
 	preserve_file_names: undefined,
 	base_dir: undefined,
 	workers_dev: undefined,
+	preview_urls: true,
 	route: undefined,
 	routes: undefined,
 	tsconfig: undefined,
diff --git a/packages/wrangler/src/config/environment.ts b/packages/wrangler/src/config/environment.ts
index dc8e88297a07..b7e95d0ec4de 100644
--- a/packages/wrangler/src/config/environment.ts
+++ b/packages/wrangler/src/config/environment.ts
@@ -148,6 +148,15 @@ interface EnvironmentInheritable {
 	 */
 	workers_dev: boolean | undefined;
 
+	/**
+	 * Whether we use <version>-<name>.<subdomain>.workers.dev to
+	 * serve Preview URLs for your Worker.
+	 *
+	 * @default `true`
+	 * @inheritable
+	 */
+	preview_urls: boolean | undefined;
+
 	/**
 	 * A list of routes that your Worker should be published to.
 	 * Only one of `routes` or `route` is required.
diff --git a/packages/wrangler/src/config/validation.ts b/packages/wrangler/src/config/validation.ts
index 87fdd56e544f..45ef783a13b6 100644
--- a/packages/wrangler/src/config/validation.ts
+++ b/packages/wrangler/src/config/validation.ts
@@ -1122,6 +1122,15 @@ function normalizeAndValidateEnvironment(
 		undefined
 	);
 
+	const preview_urls = inheritable(
+		diagnostics,
+		topLevelEnv,
+		rawEnv,
+		"preview_urls",
+		isBoolean,
+		true
+	);
+
 	const { deprecatedUpload, ...build } = normalizeAndValidateBuild(
 		diagnostics,
 		rawEnv,
@@ -1257,6 +1266,7 @@ function normalizeAndValidateEnvironment(
 		placement: normalizeAndValidatePlacement(diagnostics, topLevelEnv, rawEnv),
 		build,
 		workers_dev,
+		preview_urls,
 		// Not inherited fields
 		vars: notInheritable(
 			diagnostics,
diff --git a/packages/wrangler/src/init.ts b/packages/wrangler/src/init.ts
index e29ec4c0a8af..3efd5792681a 100644
--- a/packages/wrangler/src/init.ts
+++ b/packages/wrangler/src/init.ts
@@ -140,6 +140,7 @@ type CustomDomainsRes = {
 
 type WorkersDevRes = {
 	enabled: boolean;
+	previews_enabled: boolean;
 };
 type CronTriggersRes = {
 	schedules: [
@@ -940,6 +941,7 @@ async function getWorkerConfig(
 		name: workerName,
 		main: entrypoint,
 		workers_dev: workersDev.enabled,
+		preview_urls: workersDev.previews_enabled,
 		compatibility_date:
 			serviceEnvMetadata.script.compatibility_date ??
 			new Date().toISOString().substring(0, 10),
diff --git a/packages/wrangler/src/triggers/deploy.ts b/packages/wrangler/src/triggers/deploy.ts
index 884a517fc86d..64c46ca04bb3 100644
--- a/packages/wrangler/src/triggers/deploy.ts
+++ b/packages/wrangler/src/triggers/deploy.ts
@@ -5,7 +5,6 @@ import {
 	publishCustomDomains,
 	publishRoutes,
 	renderRoute,
-	sleep,
 	updateQueueConsumers,
 	updateQueueProducers,
 	validateRoutes,
@@ -70,8 +69,12 @@ export default async function triggersDeploy(
 		? `/accounts/${accountId}/workers/services/${scriptName}/environments/${envName}`
 		: `/accounts/${accountId}/workers/scripts/${scriptName}`;
 
-	const { enabled: available_on_subdomain } = await fetchResult<{
+	const {
+		enabled: available_on_subdomain,
+		previews_enabled: previews_available_on_subdomain,
+	} = await fetchResult<{
 		enabled: boolean;
+		previews_enabled: boolean;
 	}>(`${workerUrl}/subdomain`);
 
 	if (!props.dryRun) {
@@ -90,103 +93,110 @@ export default async function triggersDeploy(
 	const uploadMs = Date.now() - start;
 	const deployments: Promise<string[]>[] = [];
 
+	const deploymentInSync = deployToWorkersDev === available_on_subdomain;
+	const previewsInSync =
+		config.preview_urls === previews_available_on_subdomain;
+
 	if (deployToWorkersDev) {
 		// Deploy to a subdomain of `workers.dev`
 		const userSubdomain = await getWorkersDevSubdomain(
 			accountId,
 			config.configPath
 		);
-		const scriptURL =
+
+		const deploymentURL =
 			props.legacyEnv || !props.env
 				? `${scriptName}.${userSubdomain}.workers.dev`
 				: `${envName}.${scriptName}.${userSubdomain}.workers.dev`;
-		if (!available_on_subdomain) {
+
+		if (deploymentInSync && previewsInSync) {
+			deployments.push(Promise.resolve([deploymentURL]));
+		} else {
 			// Enable the `workers.dev` subdomain.
 			deployments.push(
 				fetchResult(`${workerUrl}/subdomain`, {
 					method: "POST",
-					body: JSON.stringify({ enabled: true }),
+					body: JSON.stringify({
+						enabled: true,
+						previews_enabled: config.preview_urls,
+					}),
 					headers: {
 						"Content-Type": "application/json",
 					},
-				})
-					.then(() => [scriptURL])
-					// Add a delay when the subdomain is first created.
-					// This is to prevent an issue where a negative cache-hit
-					// causes the subdomain to be unavailable for 30 seconds.
-					// This is a temporary measure until we fix this on the edge.
-					.then(async (url) => {
-						await sleep(3000);
-						return url;
-					})
+				}).then(() => [deploymentURL])
 			);
-		} else {
-			deployments.push(Promise.resolve([scriptURL]));
 		}
-	} else {
-		if (available_on_subdomain) {
-			// Disable the workers.dev deployment
-			await fetchResult(`${workerUrl}/subdomain`, {
-				method: "POST",
-				body: JSON.stringify({ enabled: false }),
-				headers: {
-					"Content-Type": "application/json",
-				},
-			});
-		} else if (routes.length !== 0) {
-			// if you get to this point it's because
-			// you're trying to deploy a worker to a custom
-			// domain that's already bound to another worker.
-			// so this thing is about finding workers that have
-			// bindings to the routes you're trying to deploy to.
-			//
-			// the logic is kinda similar (read: duplicated) from publishRoutesFallback,
-			// except here we know we have a good API token or whatever so we don't need
-			// to bother with all the error handling tomfoolery.
-			const routesWithOtherBindings: Record<string, string[]> = {};
-			for (const route of routes) {
-				const zone = await getZoneForRoute({ route, accountId });
-				if (!zone) {
-					continue;
-				}
-
-				const routePattern = typeof route === "string" ? route : route.pattern;
-				const routesInZone = await fetchListResult<{
-					pattern: string;
-					script: string;
-				}>(`/zones/${zone.id}/workers/routes`);
+	}
+	if (!deployToWorkersDev && (!deploymentInSync || !previewsInSync)) {
+		// Disable the workers.dev deployment
+		await fetchResult(`${workerUrl}/subdomain`, {
+			method: "POST",
+			body: JSON.stringify({
+				enabled: false,
+				previews_enabled: config.preview_urls,
+			}),
+			headers: {
+				"Content-Type": "application/json",
+			},
+		});
+	}
+	if (!deployToWorkersDev && deploymentInSync && routes.length !== 0) {
+		// TODO is this true? How does last subdomain status affect route confict??
+		// Why would we only need to validate route conflicts if didn't need to
+		// disable the subdomain deployment?
+
+		// if you get to this point it's because
+		// you're trying to deploy a worker to a route
+		// that's already bound to another worker.
+		// so this thing is about finding workers that have
+		// bindings to the routes you're trying to deploy to.
+		//
+		// the logic is kinda similar (read: duplicated) from publishRoutesFallback,
+		// except here we know we have a good API token or whatever so we don't need
+		// to bother with all the error handling tomfoolery.
+		const routesWithOtherBindings: Record<string, string[]> = {};
+		for (const route of routes) {
+			const zone = await getZoneForRoute({ route, accountId });
+			if (!zone) {
+				continue;
+			}
 
-				routesInZone.forEach(({ script, pattern }) => {
-					if (pattern === routePattern && script !== scriptName) {
-						if (!(script in routesWithOtherBindings)) {
-							routesWithOtherBindings[script] = [];
-						}
+			const routePattern = typeof route === "string" ? route : route.pattern;
+			const routesInZone = await fetchListResult<{
+				pattern: string;
+				script: string;
+			}>(`/zones/${zone.id}/workers/routes`);
 
-						routesWithOtherBindings[script].push(pattern);
+			routesInZone.forEach(({ script, pattern }) => {
+				if (pattern === routePattern && script !== scriptName) {
+					if (!(script in routesWithOtherBindings)) {
+						routesWithOtherBindings[script] = [];
 					}
-				});
-			}
 
-			if (Object.keys(routesWithOtherBindings).length > 0) {
-				let errorMessage =
-					"Can't deploy routes that are assigned to another worker.\n";
-
-				for (const worker in routesWithOtherBindings) {
-					const assignedRoutes = routesWithOtherBindings[worker];
-					errorMessage += `"${worker}" is already assigned to routes:\n${assignedRoutes.map(
-						(r) => `  - ${chalk.underline(r)}\n`
-					)}`;
+					routesWithOtherBindings[script].push(pattern);
 				}
+			});
+		}
 
-				const resolution =
-					"Unassign other workers from the routes you want to deploy to, and then try again.";
-				const dashHref = chalk.blue.underline(
-					`https://dash.cloudflare.com/${accountId}/workers/overview`
-				);
-				const dashLink = `Visit ${dashHref} to unassign a worker from a route.`;
+		if (Object.keys(routesWithOtherBindings).length > 0) {
+			let errorMessage =
+				"Can't deploy routes that are assigned to another worker.\n";
 
-				throw new UserError(`${errorMessage}\n${resolution}\n${dashLink}`);
+			for (const worker in routesWithOtherBindings) {
+				const assignedRoutes = routesWithOtherBindings[worker];
+				errorMessage += `"${worker}" is already assigned to routes:\n${assignedRoutes.map(
+					(r) => `  - ${chalk.underline(r)}\n`
+				)}`;
 			}
+
+			const resolution =
+				"Unassign other workers from the routes you want to deploy to, and then try again.";
+			const dashHref = chalk.blue.underline(
+				`https://dash.cloudflare.com/${accountId}/workers/overview`
+			);
+			const dashLink = `Visit ${dashHref} to unassign a worker from a route.`;
+
+			throw new UserError(`${errorMessage}\n${resolution}\n${dashLink}`);
 		}
 	}
 
diff --git a/packages/wrangler/src/versions/upload.ts b/packages/wrangler/src/versions/upload.ts
index 3b664ef1eafd..0e55160e8da7 100644
--- a/packages/wrangler/src/versions/upload.ts
+++ b/packages/wrangler/src/versions/upload.ts
@@ -570,11 +570,12 @@ See https://developers.cloudflare.com/workers/platform/compatibility-dates for m
 	let versionPreviewUrl: string | undefined = undefined;
 
 	if (versionId && hasPreview) {
-		const { enabled: available_on_subdomain } = await fetchResult<{
-			enabled: boolean;
-		}>(`${workerUrl}/subdomain`);
+		const { previews_enabled: previews_available_on_subdomain } =
+			await fetchResult<{
+				previews_enabled: boolean;
+			}>(`${workerUrl}/subdomain`);
 
-		if (available_on_subdomain) {
+		if (previews_available_on_subdomain) {
 			const userSubdomain = await getWorkersDevSubdomain(
 				accountId,
 				config.configPath