PR feedback 2

packages/workers-shared/asset-server-worker/src/global.d.ts

@@ -1,5 +1,3 @@
-type Environment = "production" | "local";
-
 type Env = {
 	// ASSETS_MANIFEST is a pipeline binding to an ArrayBuffer containing the
 	// binary-encoded site manifest
@@ -8,6 +6,4 @@ type Env = {
 	// ASSETS_KV_NAMESPACE is a pipeline binding to the KV namespace that the
 	// assets are in.
 	ASSETS_KV_NAMESPACE: KVNamespace;
-
-	ENVIRONMENT: Environment;
 };

packages/workers-shared/asset-server-worker/src/index.ts

@@ -11,7 +11,7 @@ import { getAssetWithMetadataFromKV } from "./utils/kv";
 
 export default class extends WorkerEntrypoint<Env> {
 	async fetch(request: Request) {
-		if (!request.method.match(/^(get)$/i)) {
+		if (request.method.toLowerCase() !== "get") {
 			return new MethodNotAllowedResponse();
 		}
 
@@ -22,10 +22,6 @@ export default class extends WorkerEntrypoint<Env> {
 		}
 	}
 
-	async hasResponseForRequest(request: Request) {
-		return !!(await this.getAssetEntry(request));
-	}
-
 	async handleRequest(request: Request) {
 		const assetEntry = await this.getAssetEntry(request);
 		if (!assetEntry) {
@@ -36,7 +32,7 @@ export default class extends WorkerEntrypoint<Env> {
 			this.env.ASSETS_KV_NAMESPACE,
 			assetEntry
 		);
-		console.log(assetResponse);
+
 		if (!assetResponse || !assetResponse.value) {
 			throw new Error(
 				`Requested asset ${assetEntry} exists in the asset manifest but not in the KV namespace.`

packages/workers-shared/asset-server-worker/src/responses.ts

@@ -29,13 +29,7 @@ export class MethodNotAllowedResponse extends Response {
 
 export class InternalServerErrorResponse extends Response {
 	constructor(err: Error, init?: ResponseInit) {
-		let body: string | undefined = undefined;
-		// eslint-disable-next-line @typescript-eslint/no-explicit-any
-		if ((globalThis as any).DEBUG) {
-			body = `${err.message}\n\n${err.stack}`;
-		}
-
-		super(body, {
+		super(undefined, {
 			...init,
 			status: 500,
 		});

packages/workers-shared/asset-server-worker/src/utils/headers.ts

@@ -39,7 +39,7 @@ export function getAdditionalHeaders(
 		"Content-Type": contentType,
 		"Referrer-Policy": "strict-origin-when-cross-origin",
 		"X-Content-Type-Options": "nosniff",
-		ETag: `"${assetKey}"`,
+		ETag: `${assetKey}`,
 	});
 
 	if (isCacheable(request)) {

packages/workers-shared/asset-server-worker/src/utils/kv.ts

@@ -9,7 +9,7 @@ export async function getAssetWithMetadataFromKV(
 ) {
 	let attempts = 0;
 
-	while (attempts < retries) {
+	while (attempts <= retries) {
 		try {
 			return await assetsKVNamespace.getWithMetadata<AssetMetadata>(assetKey, {
 				type: "stream",

packages/workers-shared/asset-server-worker/tests/headers.test.ts

@@ -0,0 +1,143 @@
+import { getAdditionalHeaders, getMergedHeaders } from "../src/utils/headers";
+import type { AssetMetadata } from "../src/utils/kv";
+
+describe("[Asset Worker] Response Headers", () => {
+	describe("getMergedHeaders()", () => {
+		it("should merge headers with override", () => {
+			const existingHeaders = new Headers({
+				"Accept-Encoding": "gzip",
+				"Cache-Control": "max-age=180, public",
+				"Content-Type": "text/html; charset=utf-8",
+			});
+
+			const additionalHeaders = new Headers({
+				"Accept-Encoding": "*",
+				"Content-Type": "text/javascript; charset=utf-8",
+				"Keep-Alive": "timeout=5, max=1000",
+			});
+
+			const mergedHeaders = getMergedHeaders(
+				existingHeaders,
+				additionalHeaders
+			);
+			expect(mergedHeaders).toEqual(
+				new Headers({
+					"Accept-Encoding": "*",
+					"Cache-Control": "max-age=180, public",
+					"Content-Type": "text/javascript; charset=utf-8",
+					"Keep-Alive": "timeout=5, max=1000",
+				})
+			);
+		});
+	});
+
+	describe("getAdditionalHeaders()", () => {
+		it("should return the default headers the Asset Worker should set on every response", () => {
+			const request = new Request("https://example.com", {
+				method: "GET",
+				headers: {
+					"Accept-Encoding": "*",
+				},
+			});
+			const assetMetadata: AssetMetadata = {
+				contentType: "text/html; charset=utf-8",
+			};
+			const additionalHeaders = getAdditionalHeaders(
+				"33a64df551425fcc55e4d42a148795d9f25f89d4",
+				assetMetadata,
+				request
+			);
+
+			expect(additionalHeaders).toEqual(
+				new Headers({
+					"Access-Control-Allow-Origin": "*",
+					"Content-Type": "text/html; charset=utf-8",
+					"Referrer-Policy": "strict-origin-when-cross-origin",
+					"X-Content-Type-Options": "nosniff",
+					ETag: "33a64df551425fcc55e4d42a148795d9f25f89d4",
+					"Cache-Control": "public, max-age=0, must-revalidate",
+				})
+			);
+		});
+
+		it("should default 'Content-Type' to 'application/octet-stream' if not specified by asset metadata", () => {
+			const request = new Request("https://example.com", {
+				method: "GET",
+				headers: {
+					"Accept-Encoding": "*",
+				},
+			});
+			const additionalHeaders = getAdditionalHeaders(
+				"33a64df551425fcc55e4d42a148795d9f25f89d4",
+				null,
+				request
+			);
+
+			expect(additionalHeaders).toEqual(
+				new Headers({
+					"Access-Control-Allow-Origin": "*",
+					"Content-Type": "application/octet-stream",
+					"Referrer-Policy": "strict-origin-when-cross-origin",
+					"X-Content-Type-Options": "nosniff",
+					ETag: "33a64df551425fcc55e4d42a148795d9f25f89d4",
+					"Cache-Control": "public, max-age=0, must-revalidate",
+				})
+			);
+		});
+
+		it("should set the 'charset' to 'utf-8' when appropriate, if not specified", () => {
+			const request = new Request("https://example.com", {
+				method: "GET",
+				headers: {
+					"Accept-Encoding": "*",
+				},
+			});
+			const assetMetadata: AssetMetadata = { contentType: "text/html" };
+			const additionalHeaders = getAdditionalHeaders(
+				"33a64df551425fcc55e4d42a148795d9f25f89d4",
+				assetMetadata,
+				request
+			);
+
+			expect(additionalHeaders).toEqual(
+				new Headers({
+					"Access-Control-Allow-Origin": "*",
+					"Content-Type": "text/html; charset=utf-8",
+					"Referrer-Policy": "strict-origin-when-cross-origin",
+					"X-Content-Type-Options": "nosniff",
+					ETag: "33a64df551425fcc55e4d42a148795d9f25f89d4",
+					"Cache-Control": "public, max-age=0, must-revalidate",
+				})
+			);
+		});
+
+		it("should not set the 'Cache-Control' header, if 'Authorization' and 'Range' headers are present in the request", () => {
+			const request = new Request("https://example.com", {
+				method: "GET",
+				headers: {
+					"Accept-Encoding": "*",
+					Authorization: "Basic 123",
+					Range: "bytes=0-499",
+				},
+			});
+			const assetMetadata: AssetMetadata = {
+				contentType: "text/html; charset=utf-8",
+			};
+			const additionalHeaders = getAdditionalHeaders(
+				"33a64df551425fcc55e4d42a148795d9f25f89d4",
+				assetMetadata,
+				request
+			);
+
+			expect(additionalHeaders).toEqual(
+				new Headers({
+					"Access-Control-Allow-Origin": "*",
+					"Content-Type": "text/html; charset=utf-8",
+					"Referrer-Policy": "strict-origin-when-cross-origin",
+					"X-Content-Type-Options": "nosniff",
+					ETag: "33a64df551425fcc55e4d42a148795d9f25f89d4",
+				})
+			);
+		});
+	});
+});

packages/workers-shared/asset-server-worker/tests/kv.test.ts

@@ -0,0 +1,75 @@
+import { getAssetWithMetadataFromKV } from "../src/utils/kv";
+import type { AssetMetadata } from "../src/utils/kv";
+import type { MockInstance } from "vitest";
+
+describe("[Asset Worker] Fetching assets from KV", () => {
+	describe("getAssetWithMetadataFromKV()", () => {
+		let mockKVNamespace: KVNamespace;
+		let spy: MockInstance;
+
+		beforeEach(() => {
+			mockKVNamespace = {
+				getWithMetadata: () => Promise.resolve(),
+			} as unknown as KVNamespace;
+
+			spy = vi.spyOn(mockKVNamespace, "getWithMetadata");
+		});
+
+		afterEach(() => {
+			vi.restoreAllMocks();
+		});
+
+		it("should return the asset value and metadata, if asset was found in the KV store", async () => {
+			spy.mockReturnValueOnce(
+				Promise.resolve({
+					value: "<html>Hello world</html>",
+					metadata: {
+						contentType: "text/html",
+					},
+				}) as unknown as Promise<
+					KVNamespaceGetWithMetadataResult<ReadableStream, AssetMetadata>
+				>
+			);
+
+			const asset = await getAssetWithMetadataFromKV(mockKVNamespace, "abcd");
+			expect(asset).toBeDefined();
+			expect(asset?.value).toEqual("<html>Hello world</html>");
+			expect(asset?.metadata).toEqual({
+				contentType: "text/html",
+			});
+			expect(spy).toHaveBeenCalledOnce();
+		});
+
+		it("should throw an error if something went wrong while fetching the asset", async () => {
+			spy.mockReturnValue(Promise.reject("Oeps! Something went wrong"));
+
+			await expect(() =>
+				getAssetWithMetadataFromKV(mockKVNamespace, "abcd")
+			).rejects.toThrowError(
+				"Requested asset abcd could not be fetched from KV namespace."
+			);
+		});
+
+		it("should retry once by default if something went wrong while fetching the asset", async () => {
+			spy.mockReturnValue(Promise.reject("Oeps! Something went wrong"));
+
+			await expect(() =>
+				getAssetWithMetadataFromKV(mockKVNamespace, "abcd")
+			).rejects.toThrowError(
+				"Requested asset abcd could not be fetched from KV namespace."
+			);
+			expect(spy).toHaveBeenCalledTimes(2);
+		});
+
+		it("should support custom number of retries", async () => {
+			spy.mockReturnValue(Promise.reject("Oeps! Something went wrong"));
+
+			await expect(() =>
+				getAssetWithMetadataFromKV(mockKVNamespace, "abcd", 2)
+			).rejects.toThrowError(
+				"Requested asset abcd could not be fetched from KV namespace."
+			);
+			expect(spy).toHaveBeenCalledTimes(3);
+		});
+	});
+});

packages/workers-shared/package.json

@@ -28,7 +28,8 @@
 		"check:lint": "eslint . --max-warnings=0",
 		"check:type": "tsc",
 		"clean": "rimraf dist",
-		"dev": "pnpm run clean && concurrently -n bundle:asset-server -c blue \"pnpm run bundle:asset-server --watch\""
+		"dev": "pnpm run clean && concurrently -n bundle:asset-server -c blue \"pnpm run bundle:asset-server --watch\"",
+		"test": "vitest"
 	},
 	"devDependencies": {
 		"@cloudflare/eslint-config-worker": "workspace:*",
@@ -37,7 +38,8 @@
 		"concurrently": "^8.2.2",
 		"esbuild": "0.17.19",
 		"rimraf": "^6.0.1",
-		"typescript": "^5.5.4"
+		"typescript": "^5.5.4",
+		"vitest": "2.0.5"
 	},
 	"engines": {
 		"node": ">=16.7.0"

packages/workers-shared/tsconfig.json

@@ -5,8 +5,8 @@
 		"sourceMap": true,
 		"forceConsistentCasingInFileNames": true,
 		"useUnknownInCatchVariables": false,
-		"types": ["@cloudflare/workers-types/experimental"]
+		"types": ["@cloudflare/workers-types/experimental", "vitest/globals"]
 	},
-	"include": ["**/*.ts"],
+	"include": ["**/*.ts", "vitest.config.mts"],
 	"exclude": ["node_modules", "dist"]
 }

packages/workers-shared/vitest.config.mts

@@ -0,0 +1,12 @@
+import { defineProject, mergeConfig } from "vitest/config";
+import configShared from "../../vitest.shared";
+
+export default mergeConfig(
+	configShared,
+	defineProject({
+		test: {
+			include: ["asset-server-worker/tests/**.{test,spec}.{ts,js}"],
+			globals: true,
+		},
+	})
+);