Skip to content

Commit

Permalink
fix(wrangler): prevent invalid JS identifiers in types generation (#5091
Browse files Browse the repository at this point in the history 
)

* fix(wrangler): allow dashes in types generation

* chore: add changeset

* fix: wrap LOGFWDR_SCHEMA in quotes too for consistency

* fix: check if identifier is valid for type generation

* fix: retain var values

* fix: actually retain var values

* fix: escapeStringValue tests

* chore: simplify logic

* chore: update changeset

* fix: allow $ at start of var

* fix: address issues from review

* chore: various improvements from review

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

Co-authored-by: Dario Piotrowicz <dario.piotrowicz@gmail.com>

* chore: add additional type tests for constructType

* fix: update snapshot for type generation tests

---------

Co-authored-by: Dario Piotrowicz <dario.piotrowicz@gmail.com>
  • Loading branch information
@Cherry @dario-piotrowicz
Cherry and dario-piotrowicz committed May 2, 2024
1 parent f63e7a5 commit 6365c90
Show file tree
Hide file tree
Showing 3 changed files with 165 additions and 24 deletions.
20 changes: 20 additions & 0 deletions .changeset/late-eggs-enjoy.md
View file
@@ -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";
}
```
78 changes: 76 additions & 2 deletions packages/wrangler/src/__tests__/type-generation.test.ts
View file
@@ -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"
Expand All @@ -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: [
Expand Down Expand Up @@ -205,6 +276,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;
Expand Down Expand Up @@ -374,6 +446,7 @@ describe("generateTypes()", () => {
"interface Env {
SOMETHING: \\"asdasdfasdf\\";
ANOTHER: \\"thing\\";
\\"some-other-var\\": \\"some-other-value\\";
OBJECT_VAR: {\\"enterprise\\":\\"1701-D\\",\\"activeDuty\\":true,\\"captian\\":\\"Picard\\"};
}
"
Expand Down Expand Up @@ -461,6 +534,7 @@ describe("generateTypes()", () => {
"interface CloudflareEnv {
SOMETHING: \\"asdasdfasdf\\";
ANOTHER: \\"thing\\";
\\"some-other-var\\": \\"some-other-value\\";
OBJECT_VAR: {\\"enterprise\\":\\"1701-D\\",\\"activeDuty\\":true,\\"captian\\":\\"Picard\\"};
}
"
Expand Down Expand Up @@ -547,7 +621,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]*}/
);
});

Expand Down Expand Up @@ -592,7 +666,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]*}/
);
});
});
Expand Down
91 changes: 69 additions & 22 deletions packages/wrangler/src/type-generation.ts
View file
Expand Up @@ -113,6 +113,45 @@ export async function typesHandler(
);
}

/**
* 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(
Expand Down Expand Up @@ -146,7 +185,7 @@ async function generateTypes(

if (configToDTS.kv_namespaces) {
for (const kvNamespace of configToDTS.kv_namespaces) {
envTypeStructure.push(`${kvNamespace.binding}: KVNamespace;`);
envTypeStructure.push(constructType(kvNamespace.binding, "KVNamespace"));
}
}

Expand All @@ -161,125 +200,133 @@ async function generateTypes(
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"));
}

if (configToDTS.version_metadata) {
Expand Down

0 comments on commit 6365c90

Please sign in to comment.