From 34ad3232b820fb2ab8201e391ad6f6faf0eb218f Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Thu, 23 Dec 2021 20:08:13 +0000 Subject: [PATCH] Refactor how we pass bindings from config to the worker upload api. (#163) * Refactor how we pass bindings from config to the worker upload api. There are 3 types of 'bindings' that we upload with a worker definition: `vars`, `kv_namespaces`, and `durable_objects`. These are configured in `wrangler.toml`, and get passed through to the api that uploads a worker definition (in `form_data.ts`), when using `dev` and `publish` commands. We currently serialise it as a big `variables` object, where the key of the objects defines the binding "name", and the value contains information to be bound. (https://github.com/cloudflare/wrangler2/blob/0330ecf1b54c92dfe86cb3f38394f453ed418381/packages/wrangler/src/index.tsx#L507-L530) This code sucks for many reasons: it loses some type information, hard to read/grok, hard to add new types of bindings, and mostly it's unnecessary. This PR refactors this code to instead mostly preserve the configuration style structure all the way until we actually serialise it when uploading a worker definition. I also added some more types along the way, and cleared some dead code. I will follow up this PR with 2 PRs soon: one, which introduces service bindings (as a fresh take on https://github.com/cloudflare/wrangler2/pull/156), and another one that introduces tests for configuration. * correctly pass the name of a kv namespace when binding oops. * Pass a couple of lint warnings, un-export some unused exports --- .changeset/young-buses-try.md | 5 ++ packages/wrangler/src/api/form_data.ts | 116 ++++++++++--------------- packages/wrangler/src/api/worker.ts | 62 +++++-------- packages/wrangler/src/config.ts | 10 +-- packages/wrangler/src/dev.tsx | 92 +++++++++++--------- packages/wrangler/src/index.tsx | 39 +++++---- packages/wrangler/src/publish.ts | 49 +++++------ 7 files changed, 171 insertions(+), 202 deletions(-) create mode 100644 .changeset/young-buses-try.md diff --git a/.changeset/young-buses-try.md b/.changeset/young-buses-try.md new file mode 100644 index 000000000000..1b8a431ae75d --- /dev/null +++ b/.changeset/young-buses-try.md @@ -0,0 +1,5 @@ +--- +"wrangler": patch +--- + +Refactor the way we convert configurations for bindings all the way through to the API where we upload a worker definition. This commit preserves the configuration structure (mostly) until the point we serialise it for the API. This prevents the way we use duck typing to detect a binding type when uploading, makes the types a bit simpler, and makes it easier to add other types of bindings in the future (notably, the upcoming service bindings.) diff --git a/packages/wrangler/src/api/form_data.ts b/packages/wrangler/src/api/form_data.ts index c1cc813016a7..3adf347c3756 100644 --- a/packages/wrangler/src/api/form_data.ts +++ b/packages/wrangler/src/api/form_data.ts @@ -1,72 +1,11 @@ import type { CfWorkerInit, CfModuleType, - CfVariable, CfModule, + CfDurableObjectMigrations, } from "./worker.js"; import { FormData, Blob } from "formdata-node"; -// Credit: https://stackoverflow.com/a/9458996 -function toBase64(source: BufferSource): string { - let result = ""; - const buffer = source instanceof ArrayBuffer ? source : source.buffer; - const bytes = new Uint8Array(buffer); - for (let i = 0; i < bytes.byteLength; i++) { - result += String.fromCharCode(bytes[i]); - } - return btoa(result); -} - -function toBinding( - name: string, - variable: CfVariable -): Record { - if (typeof variable === "string") { - return { name, type: "plain_text", text: variable }; - } - - if ("namespaceId" in variable) { - return { - name, - type: "kv_namespace", - namespace_id: variable.namespaceId, - }; - } - - if ("class_name" in variable) { - return { - name, - type: "durable_object_namespace", - class_name: variable.class_name, - ...(variable.script_name && { - script_name: variable.script_name, - }), - }; - } - - const { format, algorithm, usages, data } = variable; - if (format) { - let key_base64; - let key_jwk; - if (data instanceof ArrayBuffer || ArrayBuffer.isView(data)) { - key_base64 = toBase64(data); - } else { - key_jwk = data; - } - return { - name, - type: "secret_key", - format, - algorithm, - usages, - key_base64, - key_jwk, - }; - } - - throw new TypeError("Unsupported variable: " + variable); -} - export function toMimeType(type: CfModuleType): string { switch (type) { case "esm": @@ -91,6 +30,23 @@ function toModule(module: CfModule, entryType?: CfModuleType): Blob { return new Blob([content], { type }); } +interface WorkerMetadata { + compatibility_date?: string; + compatibility_flags?: string[]; + usage_model?: "bundled" | "unbound"; + migrations?: CfDurableObjectMigrations; + bindings: ( + | { type: "kv_namespace"; name: string; namespace_id: string } + | { type: "plain_text"; name: string; text: string } + | { + type: "durable_object_namespace"; + name: string; + class_name: string; + script_name?: string; + } + )[]; +} + /** * Creates a `FormData` upload from a `CfWorkerInit`. */ @@ -99,7 +55,7 @@ export function toFormData(worker: CfWorkerInit): FormData { const { main, modules, - variables, + bindings, migrations, usage_model, compatibility_date, @@ -107,16 +63,34 @@ export function toFormData(worker: CfWorkerInit): FormData { } = worker; const { name, type: mainType } = main; - const bindings = []; - for (const [name, variable] of Object.entries(variables ?? {})) { - const binding = toBinding(name, variable); - bindings.push(binding); - } + const metadataBindings: WorkerMetadata["bindings"] = []; + + bindings.kv_namespaces?.forEach(({ id, binding }) => { + metadataBindings.push({ + name: binding, + type: "kv_namespace", + namespace_id: id, + }); + }); + + bindings.durable_objects?.bindings.forEach( + ({ name, class_name, script_name }) => { + metadataBindings.push({ + name, + type: "durable_object_namespace", + class_name: class_name, + ...(script_name && { script_name }), + }); + } + ); + + Object.entries(bindings.vars || {})?.forEach(([key, value]) => { + metadataBindings.push({ name: key, type: "plain_text", text: value }); + }); - // TODO: this object should be typed - const metadata = { + const metadata: WorkerMetadata = { ...(mainType !== "commonjs" ? { main_module: name } : { body_part: name }), - bindings, + bindings: metadataBindings, ...(compatibility_date && { compatibility_date }), ...(compatibility_flags && { compatibility_flags }), ...(usage_model && { usage_model }), diff --git a/packages/wrangler/src/api/worker.ts b/packages/wrangler/src/api/worker.ts index e26fe1037711..ec101e07b28c 100644 --- a/packages/wrangler/src/api/worker.ts +++ b/packages/wrangler/src/api/worker.ts @@ -62,22 +62,31 @@ export interface CfModule { type?: CfModuleType; } +/** + * A map of variable names to values. + */ +interface CfVars { + [key: string]: string; +} + /** * A KV namespace. */ -export interface CfKvNamespace { - /** - * The namespace ID. - */ - namespaceId: string; +interface CfKvNamespace { + binding: string; + id: string; } -export interface CfDurableObject { +/** + * A Durable Object. + */ +interface CfDurableObject { + name: string; class_name: string; script_name?: string; } -interface CfDOMigrations { +export interface CfDurableObjectMigrations { old_tag?: string; new_tag: string; steps: { @@ -87,35 +96,6 @@ interface CfDOMigrations { }[]; } -/** - * A `WebCrypto` key. - * - * @link https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto/importKey - */ -export interface CfCryptoKey { - /** - * The format. - */ - format: string; - /** - * The algorithm. - */ - algorithm: string; - /** - * The usages. - */ - usages: string[]; - /** - * The data. - */ - data: BufferSource | JsonWebKey; -} - -/** - * A variable (aka. environment variable). - */ -export type CfVariable = string | CfKvNamespace | CfCryptoKey | CfDurableObject; - /** * Options for creating a `CfWorker`. */ @@ -133,10 +113,14 @@ export interface CfWorkerInit { */ modules: void | CfModule[]; /** - * The map of names to variables. (aka. environment variables) + * All the bindings */ - variables?: { [name: string]: CfVariable }; - migrations: void | CfDOMigrations; + bindings: { + kv_namespaces?: CfKvNamespace[]; + durable_objects?: { bindings: CfDurableObject[] }; + vars?: CfVars; + }; + migrations: void | CfDurableObjectMigrations; compatibility_date: string | void; compatibility_flags: void | string[]; usage_model: void | "bundled" | "unbound"; diff --git a/packages/wrangler/src/config.ts b/packages/wrangler/src/config.ts index a00de1d88edc..1af53f8114a8 100644 --- a/packages/wrangler/src/config.ts +++ b/packages/wrangler/src/config.ts @@ -1,7 +1,7 @@ // we're going to manually write both the type definition AND // the validator for the config, so that we can give better error messages -type DOMigration = { +type DurableObjectMigration = { tag: string; new_classes?: string[]; renamed_classes?: string[]; @@ -25,13 +25,13 @@ type Dev = { upstream_protocol?: string; }; -type Vars = { [key: string]: string }; +export type Vars = { [key: string]: string }; type Cron = string; // TODO: we should be able to parse a cron pattern with ts type KVNamespace = { - binding?: string; - preview_id: string; + binding: string; + preview_id?: string; id: string; }; @@ -108,7 +108,7 @@ export type Config = { jsx_factory?: string; // inherited jsx_fragment?: string; // inherited vars?: Vars; - migrations?: DOMigration[]; + migrations?: DurableObjectMigration[]; durable_objects?: { bindings: DurableObject[] }; kv_namespaces?: KVNamespace[]; site?: Site; // inherited diff --git a/packages/wrangler/src/dev.tsx b/packages/wrangler/src/dev.tsx index 16d4dfc9160b..2976b3669451 100644 --- a/packages/wrangler/src/dev.tsx +++ b/packages/wrangler/src/dev.tsx @@ -9,7 +9,7 @@ import React, { useState, useEffect, useRef } from "react"; import path from "path"; import open from "open"; import { DtInspector } from "./api/inspect"; -import type { CfModule, CfVariable } from "./api/worker"; +import type { CfModule } from "./api/worker"; import { createWorker } from "./api/worker"; import type { CfWorkerInit } from "./api/worker"; import { spawn } from "child_process"; @@ -38,7 +38,7 @@ type Props = { initialMode: "local" | "remote"; jsxFactory: void | string; jsxFragment: void | string; - variables: { [name: string]: CfVariable }; + bindings: CfWorkerInit["bindings"]; public: void | string; site: void | string; compatibilityDate: void | string; @@ -99,7 +99,7 @@ function Dev(props: Props): JSX.Element { name={props.name} bundle={bundle} format={props.format} - variables={props.variables} + bindings={props.bindings} site={props.site} public={props.public} port={props.port} @@ -111,7 +111,7 @@ function Dev(props: Props): JSX.Element { format={props.format} accountId={props.accountId} apiToken={apiToken} - variables={props.variables} + bindings={props.bindings} site={props.site} public={props.public} port={props.port} @@ -172,7 +172,7 @@ function Remote(props: { port: number; accountId: void | string; apiToken: void | string; - variables: { [name: string]: CfVariable }; + bindings: CfWorkerInit["bindings"]; compatibilityDate: string | void; compatibilityFlags: void | string[]; usageModel: void | "bundled" | "unbound"; @@ -186,7 +186,7 @@ function Remote(props: { modules: props.bundle ? props.bundle.modules : [], accountId: props.accountId, apiToken: props.apiToken, - variables: props.variables, + bindings: props.bindings, sitesFolder: props.site, port: props.port, compatibilityDate: props.compatibilityDate, @@ -203,7 +203,7 @@ function Local(props: { name: void | string; bundle: EsbuildBundle | void; format: CfScriptFormat; - variables: { [name: string]: CfVariable }; + bindings: CfWorkerInit["bindings"]; public: void | string; site: void | string; port: number; @@ -212,7 +212,7 @@ function Local(props: { name: props.name, bundle: props.bundle, format: props.format, - variables: props.variables, + bindings: props.bindings, port: props.port, }); useInspector(inspectorUrl); @@ -223,11 +223,11 @@ function useLocalWorker(props: { name: void | string; bundle: EsbuildBundle | void; format: CfScriptFormat; - variables: { [name: string]: CfVariable }; + bindings: CfWorkerInit["bindings"]; port: number; }) { // TODO: pass vars via command line - const { bundle, format, variables, port } = props; + const { bundle, format, bindings, port } = props; const local = useRef>(); const removeSignalExitListener = useRef<() => void>(); const [inspectorUrl, setInspectorUrl] = useState(); @@ -264,20 +264,17 @@ function useLocalWorker(props: { "--kv-persist", "--cache-persist", "--do-persist", - ...Object.entries(variables) - .map(([varKey, varVal]) => { - if (typeof varVal === "string") { - return `--binding ${varKey}=${varVal}`; - } else if ( - "namespaceId" in varVal && - typeof varVal.namespaceId === "string" - ) { - return `--kv ${varKey}`; - } else if ("class_name" in varVal) { - return `--do ${varKey}=${varVal.class_name}`; - } - }) - .filter(Boolean), + ...Object.entries(bindings.vars || {}).map(([key, value]) => { + return `--binding ${key}=${value}`; + }), + ...(bindings.kv_namespaces || []).map(({ binding }) => { + return `--kv ${binding}`; + }), + ...(bindings.durable_objects?.bindings || []).map( + ({ name, class_name }) => { + return `--do ${name}=${class_name}`; + } + ), "--modules", format || (bundle.type === "esm" ? "modules" : "service-worker") === "modules" @@ -338,7 +335,14 @@ function useLocalWorker(props: { removeSignalExitListener.current = undefined; } }; - }, [bundle, format, port]); + }, [ + bundle, + format, + port, + bindings.durable_objects?.bindings, + bindings.kv_namespaces, + bindings.vars, + ]); return { inspectorUrl }; } @@ -373,8 +377,6 @@ function useTmpDir(): string | void { return directory?.path; } -function runCommand() {} - function useCustomBuild( expectedEntry: string, props: { @@ -528,7 +530,7 @@ function useWorker(props: { modules: CfModule[]; accountId: string; apiToken: string; - variables: { [name: string]: CfVariable }; + bindings: CfWorkerInit["bindings"]; sitesFolder: void | string; port: number; compatibilityDate: string | void; @@ -542,7 +544,7 @@ function useWorker(props: { modules, accountId, apiToken, - variables, + bindings, sitesFolder, compatibilityDate, compatibilityFlags, @@ -591,19 +593,23 @@ function useWorker(props: { type: format || bundle.type === "esm" ? "esm" : "commonjs", content, }, - modules: assets.manifest - ? modules.concat({ - name: "__STATIC_CONTENT_MANIFEST", - content: JSON.stringify(assets.manifest), - type: "text", - }) - : modules, - variables: assets.namespace - ? { - ...variables, - __STATIC_CONTENT: { namespaceId: assets.namespace }, - } - : variables, + modules: modules.concat( + assets.manifest + ? { + name: "__STATIC_CONTENT_MANIFEST", + content: JSON.stringify(assets.manifest), + type: "text", + } + : [] + ), + bindings: { + ...bindings, + kv_namespaces: (bindings.kv_namespaces || []).concat( + assets.namespace + ? { binding: "__STATIC_CONTENT", id: assets.namespace } + : [] + ), + }, migrations: undefined, // no migrations in dev compatibility_date: compatibilityDate, compatibility_flags: compatibilityFlags, @@ -633,6 +639,8 @@ function useWorker(props: { compatibilityDate, compatibilityFlags, usageModel, + bindings, + modules, ]); return token; } diff --git a/packages/wrangler/src/index.tsx b/packages/wrangler/src/index.tsx index 892eac2d5c5b..cc707993d018 100644 --- a/packages/wrangler/src/index.tsx +++ b/packages/wrangler/src/index.tsx @@ -504,27 +504,30 @@ export async function main(argv: string[]): Promise { compatibilityDate={config.compatibility_date} compatibilityFlags={config.compatibility_flags} usageModel={config.usage_model} - variables={{ - ...(envRootObj?.vars || {}), - ...(envRootObj?.kv_namespaces || []).reduce( - (obj, { binding, preview_id }) => { + bindings={{ + kv_namespaces: envRootObj.kv_namespaces?.map( + ({ binding, preview_id, id: _id }) => { + // In `dev`, we make folks use a separate kv namespace called + // `preview_id` instead of `id` so that they don't + // break production data. So here we check that a `preview_id` + // has actually been configured. + // This whole block of code will be obsolted in the future + // when we have copy-on-write for previews on edge workers. if (!preview_id) { // TODO: This error has to be a _lot_ better, ideally just asking // to create a preview namespace for the user automatically throw new Error( - "kv namespaces need a preview id during dev mode" - ); + `In development, you should use a separate kv namespace than the one you'd use in production. Please create a new kv namespace with "wrangler kv:namespace create --preview" and add its id as preview_id to the kv_namespace "${binding}" in your wrangler.toml` + ); // Ugh, I really don't like this message very much } - return { ...obj, [binding]: { namespaceId: preview_id } }; - }, - {} - ), - ...(envRootObj?.durable_objects?.bindings || []).reduce( - (obj, { name, class_name, script_name }) => { - return { ...obj, [name]: { class_name, script_name } }; - }, - {} + return { + binding, + id: preview_id, + }; + } ), + vars: envRootObj.vars, + durable_objects: envRootObj.durable_objects, }} /> ); @@ -957,7 +960,11 @@ export async function main(argv: string[]): Promise { content: `export default { fetch() {} }`, type: "esm", }, - variables: {}, + bindings: { + kv_namespaces: [], + vars: {}, + durable_objects: { bindings: [] }, + }, modules: [], }), } diff --git a/packages/wrangler/src/publish.ts b/packages/wrangler/src/publish.ts index de8b523597c2..81e2daecd24c 100644 --- a/packages/wrangler/src/publish.ts +++ b/packages/wrangler/src/publish.ts @@ -166,7 +166,7 @@ export default async function publish(props: Props): Promise { ); if (foundIndex === -1) { console.warn( - `The published script ${scriptName} has a migration tag "${script.migration_tag}, which was not found in wrangler.toml. You may have already delated it. Applying all available migrations to the script...` + `The published script ${scriptName} has a migration tag "${script.migration_tag}, which was not found in wrangler.toml. You may have already deleted it. Applying all available migrations to the script...` ); migrations = { old_tag: script.migration_tag, @@ -201,6 +201,15 @@ export default async function publish(props: Props): Promise { : { manifest: undefined, namespace: undefined }; const envRootObj = props.env ? config.env[props.env] || {} : config; + const bindings: CfWorkerInit["bindings"] = { + kv_namespaces: envRootObj.kv_namespaces?.concat( + assets.namespace + ? { binding: "__STATIC_CONTENT", id: assets.namespace } + : [] + ), + vars: envRootObj.vars, + durable_objects: envRootObj.durable_objects, + }; const worker: CfWorkerInit = { name: scriptName, @@ -209,35 +218,17 @@ export default async function publish(props: Props): Promise { content: content, type: bundle.type === "esm" ? "esm" : "commonjs", }, - variables: { - ...(envRootObj?.vars || {}), - ...(envRootObj?.kv_namespaces || []).reduce( - (obj, { binding, preview_id: _preview_id, id }) => { - return { ...obj, [binding]: { namespaceId: id } }; - }, - {} - ), - ...(envRootObj?.durable_objects?.bindings || []).reduce( - (obj, { name, class_name, script_name }) => { - return { - ...obj, - [name]: { class_name, ...(script_name && { script_name }) }, - }; - }, - {} - ), - ...(assets.namespace - ? { __STATIC_CONTENT: { namespaceId: assets.namespace } } - : {}), - }, + bindings, ...(migrations && { migrations }), - modules: assets.manifest - ? moduleCollector.modules.concat({ - name: "__STATIC_CONTENT_MANIFEST", - content: JSON.stringify(assets.manifest), - type: "text", - }) - : moduleCollector.modules, + modules: moduleCollector.modules.concat( + assets.manifest + ? { + name: "__STATIC_CONTENT_MANIFEST", + content: JSON.stringify(assets.manifest), + type: "text", + } + : [] + ), compatibility_date: config.compatibility_date, compatibility_flags: config.compatibility_flags, usage_model: config.usage_model,