PR review fixes

Co-authored-by: Greg Brimble <[email protected]>

fixtures/workers-with-assets/wrangler.toml

@@ -2,4 +2,4 @@ name = "assets-worker"
 compatibility_date = "2024-01-01"
 
 [experimental_assets]
-directory = "./public"
+directory = "./public"

packages/workers-shared/README.md

@@ -9,5 +9,5 @@ The Asset Server Worker.
 For more details please refer to the dedicated README file.
 
 > [!NOTE]
-> Since code in this package is used by the Pages infrastructure, it is important that PRs are given careful review with regards to how they could cause a failure in production.
+> Since code in this package is used by the Workers infrastructure, it is important that PRs are given careful review with regards to how they could cause a failure in production.
 > Ideally, there should be comprehensive tests for changes being made to give extra confidence about the behavior.

packages/workers-shared/asset-server-worker/wrangler.toml

@@ -1,3 +1,12 @@
+##
+# Configuration file for the Asset Server Worker
+#
+# Please note that wrangler has a dependency on this file, and will
+# attempt to read it as part of setting up a new Miniflare instance
+# in developemnt mode. We should ensure that any configuration changes
+# to this file are persisted in wrangler as well, when necessary.
+# (see packages/wrangler/src/dev/miniflare.ts -> buildMiniflareOptions())
+##
 name = "asset-server"
 main = "src/index.ts"
-compatibility_date = "2021-11-02" # Oldest version of the runtime
+compatibility_date = "2024-07-31"

packages/wrangler/src/__tests__/dev.test.tsx

@@ -1303,7 +1303,7 @@ describe("wrangler dev", () => {
 			await expect(
 				runWrangler("dev --legacy-assets abc --site xyz")
 			).rejects.toThrowErrorMatchingInlineSnapshot(
-				`[Error: Cannot use Assets and Workers Sites in the same Worker.]`
+				`[Error: Cannot use Legacy Assets and Workers Sites in the same Worker.]`
 			);
 		});
 
@@ -1318,7 +1318,7 @@ describe("wrangler dev", () => {
 			await expect(
 				runWrangler("dev --legacy-assets abc")
 			).rejects.toThrowErrorMatchingInlineSnapshot(
-				`[Error: Cannot use Assets and Workers Sites in the same Worker.]`
+				`[Error: Cannot use Legacy Assets and Workers Sites in the same Worker.]`
 			);
 		});
 
@@ -1331,7 +1331,7 @@ describe("wrangler dev", () => {
 			await expect(
 				runWrangler("dev --site xyz")
 			).rejects.toThrowErrorMatchingInlineSnapshot(
-				`[Error: Cannot use Assets and Workers Sites in the same Worker.]`
+				`[Error: Cannot use Legacy Assets and Workers Sites in the same Worker.]`
 			);
 		});
 
@@ -1347,7 +1347,7 @@ describe("wrangler dev", () => {
 			await expect(
 				runWrangler("dev --legacy-assets abc")
 			).rejects.toThrowErrorMatchingInlineSnapshot(
-				`[Error: Cannot use Assets and Workers Sites in the same Worker.]`
+				`[Error: Cannot use Legacy Assets and Workers Sites in the same Worker.]`
 			);
 		});
 
@@ -1441,11 +1441,11 @@ describe("wrangler dev", () => {
 			await expect(
 				runWrangler("dev")
 			).rejects.toThrowErrorMatchingInlineSnapshot(
-				`[Error: Cannot use Assets and Workers Sites in the same Worker.]`
+				`[Error: Cannot use Experimental Assets and Workers Sites in the same Worker.]`
 			);
 		});
 
-		it("should error if --experimental-assets and config.site are used together", async () => {
+		it("should error if config.site and --experimental-assets are used together", async () => {
 			writeWranglerToml({
 				main: "./index.js",
 				site: {
@@ -1457,7 +1457,74 @@ describe("wrangler dev", () => {
 			await expect(
 				runWrangler("dev --experimental-assets assets")
 			).rejects.toThrowErrorMatchingInlineSnapshot(
-				`[Error: Cannot use Assets and Workers Sites in the same Worker.]`
+				`[Error: Cannot use Experimental Assets and Workers Sites in the same Worker.]`
+			);
+		});
+
+		it("should error if config.experimental_assets and config.legacy_assets are used together", async () => {
+			writeWranglerToml({
+				main: "./index.js",
+				experimental_assets: { directory: "assets" },
+				legacy_assets: {
+					bucket: "xyz",
+					include: [],
+					exclude: [],
+					browser_TTL: undefined,
+					serve_single_page_app: true,
+				},
+			});
+			fs.writeFileSync("index.js", `export default {};`);
+			fs.openSync("assets", "w");
+			await expect(
+				runWrangler("dev")
+			).rejects.toThrowErrorMatchingInlineSnapshot(
+				`[Error: Cannot use Legacy Assets and Experimental Assets in the same Worker.]`
+			);
+		});
+
+		it("should error if --experimental-assets and --legacy-assets are used together", async () => {
+			fs.writeFileSync("index.js", `export default {};`);
+			fs.openSync("assets", "w");
+			await expect(
+				runWrangler("dev --experimental-assets assets --legacy-assets assets")
+			).rejects.toThrowErrorMatchingInlineSnapshot(
+				`[Error: Cannot use Legacy Assets and Experimental Assets in the same Worker.]`
+			);
+		});
+
+		it("should error if --experimental-assets and config.legacy_assets are used together", async () => {
+			writeWranglerToml({
+				main: "./index.js",
+				legacy_assets: {
+					bucket: "xyz",
+					include: [],
+					exclude: [],
+					browser_TTL: undefined,
+					serve_single_page_app: true,
+				},
+			});
+			fs.writeFileSync("index.js", `export default {};`);
+			fs.openSync("assets", "w");
+			await expect(
+				runWrangler("dev --experimental-assets assets")
+			).rejects.toThrowErrorMatchingInlineSnapshot(
+				`[Error: Cannot use Legacy Assets and Experimental Assets in the same Worker.]`
+			);
+		});
+
+		it("should error if config.experimental_assets and --legacy-assets are used together", async () => {
+			writeWranglerToml({
+				main: "./index.js",
+				experimental_assets: {
+					directory: "xyz",
+				},
+			});
+			fs.writeFileSync("index.js", `export default {};`);
+			fs.openSync("xyz", "w");
+			await expect(
+				runWrangler("dev --legacy-assets xyz")
+			).rejects.toThrowErrorMatchingInlineSnapshot(
+				`[Error: Cannot use Legacy Assets and Experimental Assets in the same Worker.]`
 			);
 		});
 
@@ -1475,7 +1542,7 @@ describe("wrangler dev", () => {
 			);
 		});
 
-		it("should error if directory specified by 'experimental_assets' configuration key does not exist", async () => {
+		it("should error if directory specified by '[experimental_assets]' configuration key does not exist", async () => {
 			writeWranglerToml({
 				main: "./index.js",
 				experimental_assets: {

packages/wrangler/src/deployment-bundle/entry.ts

@@ -45,6 +45,7 @@ export async function getEntry(
 ): Promise<Entry> {
 	let file: string;
 	let directory = process.cwd();
+
 	if (args.script) {
 		// If the script name comes from the command line it is relative to the current working directory.
 		file = path.resolve(args.script);

packages/wrangler/src/dev.tsx

@@ -562,6 +562,7 @@ export async function startDev(args: StartDevOptions) {
 				"--local is no longer required and will be removed in a future version.\n`wrangler dev` now uses the local Cloudflare Workers runtime by default. 🎉"
 			);
 		}
+
 		if (args.experimentalLocal) {
 			logger.warn(
 				"--experimental-local is no longer required and will be removed in a future version.\n`wrangler dev` now uses the local Cloudflare Workers runtime by default. 🎉"
@@ -574,6 +575,7 @@ export async function startDev(args: StartDevOptions) {
 				"Passing --inspect is unnecessary, now you can always connect to devtools."
 			);
 		}
+
 		if (args.experimentalPublic) {
 			throw new UserError(
 				"The --experimental-public field has been deprecated, try --legacy-assets instead."
@@ -599,6 +601,16 @@ export async function startDev(args: StartDevOptions) {
 			args.forceLocal = true;
 		}
 
+		/*
+		 * - `config.legacy_assets` conflates `legacy_assets` and `assets`
+		 * - `args.legacyAssets` conflates `legacy-assets` and `assets`
+		 */
+		if ((args.legacyAssets || config.legacy_assets) && experimentalAssets) {
+			throw new UserError(
+				"Cannot use Legacy Assets and Experimental Assets in the same Worker."
+			);
+		}
+
 		const projectRoot = configPath && path.dirname(configPath);
 
 		const devEnv = new DevEnv();
@@ -1184,15 +1196,25 @@ export async function validateDevServerSettings(
 	args: StartDevOptions,
 	config: Config
 ) {
+	/*
+	 * - `args.legacyAssets` conflates `legacy-assets` and `assets`
+	 * - `config.legacy_assets` conflates `legacy_assets` and `assets`
+	 */
+	if (
+		(args.legacyAssets || config.legacy_assets) &&
+		(args.site || config.site)
+	) {
+		throw new UserError(
+			"Cannot use Legacy Assets and Workers Sites in the same Worker."
+		);
+	}
+
 	if (
-		(args.legacyAssets ||
-			config.legacy_assets ||
-			args.experimentalAssets ||
-			config.experimental_assets) &&
+		(args.experimentalAssets || config.experimental_assets) &&
 		(args.site || config.site)
 	) {
 		throw new UserError(
-			"Cannot use Assets and Workers Sites in the same Worker."
+			"Cannot use Experimental Assets and Workers Sites in the same Worker."
 		);
 	}
 

packages/wrangler/src/dev/miniflare.ts

@@ -853,15 +853,16 @@ export async function buildMiniflareOptions(
 	try {
 		assetServerConfig = readConfig(assetServerConfigPath, {});
 	} catch (err) {
-		//  ignore and continue with some predefined default config
+		throw new UserError(
+			"Failed to read the Asset Server Worker configuration file.\n" + `${err}`
+		);
 	}
 
 	const assetServerWorker: WorkerOptions | undefined = config.experimentalAssets
 		? {
-				name: assetServerConfig?.name ?? "asset-server",
-				compatibilityDate:
-					assetServerConfig?.compatibility_date ?? "2024-01-01",
-				compatibilityFlags: assetServerConfig?.compatibility_flags ?? [],
+				name: assetServerConfig?.name,
+				compatibilityDate: assetServerConfig?.compatibility_date,
+				compatibilityFlags: assetServerConfig?.compatibility_flags,
 				modulesRoot: dirname(assetServerModulePath),
 				modules: [
 					{

packages/wrangler/templates/no-op-assets-worker.ts

@@ -28,7 +28,6 @@ type Env = {
 
 export default {
 	async fetch(request: Request, env: Env) {
-		const response = await env.ASSET_SERVER.fetch(request.url, request);
-		return new Response(response.body, response);
+		return env.ASSET_SERVER.fetch(request);
 	},
 };