fix(wrangler): prevent invalid JS identifiers in types generation

.changeset/late-eggs-enjoy.md

@@ -0,0 +1,20 @@
+---
+"wrangler": patch
+---
+
+fix: better handle dashes and other invalid JS identifier characters in `wrangler types` generation for vars, bindings, etc.
+
+Previously, with the following in your `wrangler.toml`, an invalid types file would be generated:
+
+```toml
+[vars]
+some-var = "foobar"
+```
+
+Now, the generated types file will be valid:
+
+```typescript
+interface Env {
+	"some-var": "foobar";
+}
+```

packages/wrangler/src/__tests__/type-generation.test.ts

@@ -1,11 +1,81 @@
 import * as fs from "fs";
 import * as TOML from "@iarna/toml";
+import {
+	constructType,
+	constructTypeKey,
+	isValidIdentifier,
+} from "../type-generation";
 import { dedent } from "../utils/dedent";
 import { mockConsoleMethods } from "./helpers/mock-console";
 import { runInTempDir } from "./helpers/run-in-tmp";
 import { runWrangler } from "./helpers/run-wrangler";
 import type { EnvironmentNonInheritable } from "../config/environment";
 
+describe("isValidIdentifier", () => {
+	it("should return true for valid identifiers", () => {
+		expect(isValidIdentifier("valid")).toBe(true);
+		expect(isValidIdentifier("valid123")).toBe(true);
+		expect(isValidIdentifier("valid_123")).toBe(true);
+		expect(isValidIdentifier("valid_123_")).toBe(true);
+		expect(isValidIdentifier("_valid_123_")).toBe(true);
+		expect(isValidIdentifier("_valid_123_")).toBe(true);
+		expect(isValidIdentifier("$valid")).toBe(true);
+		expect(isValidIdentifier("$valid$")).toBe(true);
+	});
+
+	it("should return false for invalid identifiers", () => {
+		expect(isValidIdentifier("123invalid")).toBe(false);
+		expect(isValidIdentifier("invalid-123")).toBe(false);
+		expect(isValidIdentifier("invalid 123")).toBe(false);
+	});
+});
+
+describe("constructTypeKey", () => {
+	it("should return a valid type key", () => {
+		expect(constructTypeKey("valid")).toBe("valid");
+		expect(constructTypeKey("valid123")).toBe("valid123");
+		expect(constructTypeKey("valid_123")).toBe("valid_123");
+		expect(constructTypeKey("valid_123_")).toBe("valid_123_");
+		expect(constructTypeKey("_valid_123_")).toBe("_valid_123_");
+		expect(constructTypeKey("_valid_123_")).toBe("_valid_123_");
+		expect(constructTypeKey("$valid")).toBe("$valid");
+		expect(constructTypeKey("$valid$")).toBe("$valid$");
+
+		expect(constructTypeKey("123invalid")).toBe('"123invalid"');
+		expect(constructTypeKey("invalid-123")).toBe('"invalid-123"');
+		expect(constructTypeKey("invalid 123")).toBe('"invalid 123"');
+	});
+});
+
+describe("constructType", () => {
+	it("should return a valid type", () => {
+		expect(constructType("valid", "string")).toBe("valid: string;");
+		expect(constructType("valid123", "string")).toBe("valid123: string;");
+		expect(constructType("valid_123", "string")).toBe("valid_123: string;");
+		expect(constructType("valid_123_", "string")).toBe("valid_123_: string;");
+		expect(constructType("_valid_123_", "string")).toBe("_valid_123_: string;");
+		expect(constructType("_valid_123_", "string")).toBe("_valid_123_: string;");
+
+		expect(constructType("123invalid", "string")).toBe('"123invalid": string;');
+		expect(constructType("invalid-123", "string")).toBe(
+			'"invalid-123": string;'
+		);
+		expect(constructType("invalid 123", "string")).toBe(
+			'"invalid 123": string;'
+		);
+
+		expect(constructType("valid", 'a"', false)).toBe('valid: "a\\"";');
+		expect(constructType("valid", "a\\", false)).toBe('valid: "a\\";');
+		expect(constructType("valid", "a\\b", false)).toBe('valid: "a\\b";');
+		expect(constructType("valid", 'a\\b"', false)).toBe('valid: "a\\b\\"";');
+
+		expect(constructType("valid", 1)).toBe("valid: 1;");
+		expect(constructType("valid", 12345)).toBe("valid: 12345;");
+		expect(constructType("valid", true)).toBe("valid: true;");
+		expect(constructType("valid", false)).toBe("valid: false;");
+	});
+});
+
 const bindingsConfigMock: Omit<
 	EnvironmentNonInheritable,
 	"define" | "tail_consumers" | "constellation" | "cloudchamber"
@@ -20,6 +90,7 @@ const bindingsConfigMock: Omit<
 			activeDuty: true,
 			captian: "Picard",
 		}, // We can assume the objects will be stringified
+		"some-other-var": "some-other-value",
 	},
 	queues: {
 		producers: [
@@ -202,6 +273,7 @@ describe("generateTypes()", () => {
 			TEST_KV_NAMESPACE: KVNamespace;
 			SOMETHING: \\"asdasdfasdf\\";
 			ANOTHER: \\"thing\\";
+			\\"some-other-var\\": \\"some-other-value\\";
 			OBJECT_VAR: {\\"enterprise\\":\\"1701-D\\",\\"activeDuty\\":true,\\"captian\\":\\"Picard\\"};
 			DURABLE_TEST1: DurableObjectNamespace;
 			DURABLE_TEST2: DurableObjectNamespace;
@@ -370,6 +442,7 @@ describe("generateTypes()", () => {
 		"interface Env {
 			SOMETHING: \\"asdasdfasdf\\";
 			ANOTHER: \\"thing\\";
+			\\"some-other-var\\": \\"some-other-value\\";
 			OBJECT_VAR: {\\"enterprise\\":\\"1701-D\\",\\"activeDuty\\":true,\\"captian\\":\\"Picard\\"};
 		}
 		"
@@ -457,6 +530,7 @@ describe("generateTypes()", () => {
 			"interface CloudflareEnv {
 				SOMETHING: \\"asdasdfasdf\\";
 				ANOTHER: \\"thing\\";
+				\\"some-other-var\\": \\"some-other-value\\";
 				OBJECT_VAR: {\\"enterprise\\":\\"1701-D\\",\\"activeDuty\\":true,\\"captian\\":\\"Picard\\"};
 			}
 			"
@@ -543,7 +617,7 @@ describe("generateTypes()", () => {
 				expect(fs.existsSync("./worker-configuration.d.ts")).toBe(false);
 
 				expect(fs.readFileSync("./cloudflare-env.d.ts", "utf-8")).toMatch(
-					/interface Env \{[\s\S]*SOMETHING: "asdasdfasdf";[\s\S]*ANOTHER: "thing";[\s\S]*OBJECT_VAR: \{"enterprise":"1701-D","activeDuty":true,"captian":"Picard"\};[\s\S]*}/
+					/interface Env \{[\s\S]*SOMETHING: "asdasdfasdf";[\s\S]*ANOTHER: "thing";[\s\S]*"some-other-var": "some-other-value";[\s\S]*OBJECT_VAR: \{"enterprise":"1701-D","activeDuty":true,"captian":"Picard"\};[\s\S]*}/
 				);
 			});
 
@@ -588,7 +662,7 @@ describe("generateTypes()", () => {
 			expect(
 				fs.readFileSync("./my-cloudflare-env-interface.d.ts", "utf-8")
 			).toMatch(
-				/interface MyCloudflareEnvInterface \{[\s\S]*SOMETHING: "asdasdfasdf";[\s\S]*ANOTHER: "thing";[\s\S]*OBJECT_VAR: \{"enterprise":"1701-D","activeDuty":true,"captian":"Picard"\};[\s\S]*}/
+				/interface MyCloudflareEnvInterface \{[\s\S]*SOMETHING: "asdasdfasdf";[\s\S]*ANOTHER: "thing";[\s\S]*"some-other-var": "some-other-value";[\s\S]*OBJECT_VAR: \{"enterprise":"1701-D","activeDuty":true,"captian":"Picard"\};[\s\S]*}/
 			);
 		});
 	});

packages/wrangler/src/type-generation.ts

@@ -112,6 +112,45 @@
 	);
 }
 
+/**
+ * Check if a string is a valid TypeScript identifier. This is a naive check and doesn't cover all cases
+ */
+export function isValidIdentifier(key: string) {
+	return /^[a-zA-Z_$][\w$]*$/.test(key);
+}
+
+/**
+ * Construct a type key, if it's not a valid identifier, wrap it in quotes
+ */
+export function constructTypeKey(key: string) {
+	if (isValidIdentifier(key)) {
+		return `${key}`;
+	}
+	return `"${key}"`;
+}
+
+/**
+ * Construct a full type definition for a key-value pair
+ * If useRawVal is true, we'll use the value as the type, otherwise we'll format it for a string definition
+ */
+export function constructType(
+	key: string,
+	value: string | number | boolean,
+	useRawVal = true
+) {
+	const typeKey = constructTypeKey(key);
+	if (typeof value === "string") {
+		if (useRawVal) {
+			return `${typeKey}: ${value};`;
+		}
+		return `${typeKey}: "${value.replace(/"/g, '\\"')}";`;
+	}
+	if (typeof value === "number" || typeof value === "boolean") {
+		return `${typeKey}: ${value};`;
+	}
+	return `${typeKey}: unknown;`;
+}
+
 type Secrets = Record<string, string>;
 
 async function generateTypes(
@@ -145,7 +184,7 @@
 
 	if (configToDTS.kv_namespaces) {
 		for (const kvNamespace of configToDTS.kv_namespaces) {
-			envTypeStructure.push(`${kvNamespace.binding}: KVNamespace;`);
+			envTypeStructure.push(constructType(kvNamespace.binding, "KVNamespace"));
 		}
 	}
 
@@ -160,125 +199,133 @@
 				typeof varValue === "number" ||
 				typeof varValue === "boolean"
 			) {
-				envTypeStructure.push(`${varName}: "${varValue}";`);
+				envTypeStructure.push(constructType(varName, varValue, false));
 			}
 			if (typeof varValue === "object" && varValue !== null) {
-				envTypeStructure.push(`${varName}: ${JSON.stringify(varValue)};`);
+				envTypeStructure.push(
+					constructType(varName, JSON.stringify(varValue), true)
+				);
 			}
 		}
 	}
 
 	for (const secretName in configToDTS.secrets) {
-		envTypeStructure.push(`${secretName}: string;`);
+		envTypeStructure.push(constructType(secretName, "string", true));
 	}
 
 	if (configToDTS.durable_objects?.bindings) {
 		for (const durableObject of configToDTS.durable_objects.bindings) {
-			envTypeStructure.push(`${durableObject.name}: DurableObjectNamespace;`);
+			envTypeStructure.push(
+				constructType(durableObject.name, "DurableObjectNamespace")
+			);
 		}
 	}
 
 	if (configToDTS.r2_buckets) {
 		for (const R2Bucket of configToDTS.r2_buckets) {
-			envTypeStructure.push(`${R2Bucket.binding}: R2Bucket;`);
+			envTypeStructure.push(constructType(R2Bucket.binding, "R2Bucket"));
 		}
 	}
 
 	if (configToDTS.d1_databases) {
 		for (const d1 of configToDTS.d1_databases) {
-			envTypeStructure.push(`${d1.binding}: D1Database;`);
+			envTypeStructure.push(constructType(d1.binding, "D1Database"));
 		}
 	}
 
 	if (configToDTS.services) {
 		for (const service of configToDTS.services) {
-			envTypeStructure.push(`${service.binding}: Fetcher;`);
+			envTypeStructure.push(constructType(service.binding, "Fetcher"));
 		}
 	}
 
 	if (configToDTS.constellation) {
 		for (const service of configToDTS.constellation) {
-			envTypeStructure.push(`${service.binding}: Fetcher;`);
+			envTypeStructure.push(constructType(service.binding, "Fetcher"));
 		}
 	}
 
 	if (configToDTS.analytics_engine_datasets) {
 		for (const analyticsEngine of configToDTS.analytics_engine_datasets) {
 			envTypeStructure.push(
-				`${analyticsEngine.binding}: AnalyticsEngineDataset;`
+				constructType(analyticsEngine.binding, "AnalyticsEngineDataset")
 			);
 		}
 	}
 
 	if (configToDTS.dispatch_namespaces) {
 		for (const namespace of configToDTS.dispatch_namespaces) {
-			envTypeStructure.push(`${namespace.binding}: DispatchNamespace;`);
+			envTypeStructure.push(
+				constructType(namespace.binding, "DispatchNamespace")
+			);
 		}
 	}
 
 	if (configToDTS.logfwdr?.bindings?.length) {
-		envTypeStructure.push(`LOGFWDR_SCHEMA: any;`);
+		envTypeStructure.push(constructType("LOGFWDR_SCHEMA", "any"));
 	}
 
 	if (configToDTS.data_blobs) {
 		for (const dataBlobs in configToDTS.data_blobs) {
-			envTypeStructure.push(`${dataBlobs}: ArrayBuffer;`);
+			envTypeStructure.push(constructType(dataBlobs, "ArrayBuffer"));
 		}
 	}
 
 	if (configToDTS.text_blobs) {
 		for (const textBlobs in configToDTS.text_blobs) {
-			envTypeStructure.push(`${textBlobs}: string;`);
+			envTypeStructure.push(constructType(textBlobs, "string"));
 		}
 	}
 
 	if (configToDTS.unsafe?.bindings) {
 		for (const unsafe of configToDTS.unsafe.bindings) {
-			envTypeStructure.push(`${unsafe.name}: any;`);
+			envTypeStructure.push(constructType(unsafe.name, "any"));
 		}
 	}
 
 	if (configToDTS.queues) {
 		if (configToDTS.queues.producers) {
 			for (const queue of configToDTS.queues.producers) {
-				envTypeStructure.push(`${queue.binding}: Queue;`);
+				envTypeStructure.push(constructType(queue.binding, "Queue"));
 			}
 		}
 	}
 
 	if (configToDTS.send_email) {
 		for (const sendEmail of configToDTS.send_email) {
-			envTypeStructure.push(`${sendEmail.name}: SendEmail;`);
+			envTypeStructure.push(constructType(sendEmail.name, "SendEmail"));
 		}
 	}
 
 	if (configToDTS.vectorize) {
 		for (const vectorize of configToDTS.vectorize) {
-			envTypeStructure.push(`${vectorize.binding}: VectorizeIndex;`);
+			envTypeStructure.push(constructType(vectorize.binding, "VectorizeIndex"));
 		}
 	}
 
 	if (configToDTS.hyperdrive) {
 		for (const hyperdrive of configToDTS.hyperdrive) {
-			envTypeStructure.push(`${hyperdrive.binding}: Hyperdrive;`);
+			envTypeStructure.push(constructType(hyperdrive.binding, "Hyperdrive"));
 		}
 	}
 
 	if (configToDTS.mtls_certificates) {
 		for (const mtlsCertificate of configToDTS.mtls_certificates) {
-			envTypeStructure.push(`${mtlsCertificate.binding}: Fetcher;`);
+			envTypeStructure.push(constructType(mtlsCertificate.binding, "Fetcher"));
 		}
 	}
 
 	if (configToDTS.browser) {
 		// The BrowserWorker type in @cloudflare/puppeteer is of type
 		// { fetch: typeof fetch }, but workers-types doesn't include it
 		// and Fetcher is valid for the purposes of handing it to puppeteer
-		envTypeStructure.push(`${configToDTS.browser.binding}: Fetcher;`);
+		envTypeStructure.push(
+			constructType(configToDTS.browser.binding, "Fetcher")
+		);
 	}
 
 	if (configToDTS.ai) {
-		envTypeStructure.push(`${configToDTS.ai.binding}: unknown;`);
+		envTypeStructure.push(constructType(configToDTS.ai.binding, "unknown"));
 	}
 
 	const modulesTypeStructure: string[] = [];