From b40d0ab4fdd3a03c06ebb7682e4eea0e561afe81 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Wed, 4 Dec 2024 11:43:52 +0000 Subject: [PATCH] fix: allow the asset directory to be omitted in Wrangler config for commands that don't need it (#7426) --- .changeset/forty-gifts-grab.md | 5 ++ .../src/__tests__/configuration.test.ts | 22 -------- .../wrangler/src/__tests__/deploy.test.ts | 54 ++++++++++++++++++- .../src/api/integrations/platform/index.ts | 4 +- .../api/startDevWorker/ConfigController.ts | 4 +- packages/wrangler/src/assets.ts | 25 ++++++--- packages/wrangler/src/config/environment.ts | 9 +++- .../wrangler/src/config/validation-helpers.ts | 21 -------- packages/wrangler/src/config/validation.ts | 11 +--- packages/wrangler/src/deploy/index.ts | 4 +- packages/wrangler/src/triggers/index.ts | 4 +- packages/wrangler/src/versions/index.ts | 4 +- 12 files changed, 96 insertions(+), 71 deletions(-) create mode 100644 .changeset/forty-gifts-grab.md diff --git a/.changeset/forty-gifts-grab.md b/.changeset/forty-gifts-grab.md new file mode 100644 index 000000000000..e1881f936d2e --- /dev/null +++ b/.changeset/forty-gifts-grab.md @@ -0,0 +1,5 @@ +--- +"wrangler": patch +--- + +fix: allow the asset directory to be omitted in Wrangler config for commands that don't need it diff --git a/packages/wrangler/src/__tests__/configuration.test.ts b/packages/wrangler/src/__tests__/configuration.test.ts index a6991ba6513e..38ba0bdf5c1c 100644 --- a/packages/wrangler/src/__tests__/configuration.test.ts +++ b/packages/wrangler/src/__tests__/configuration.test.ts @@ -1792,7 +1792,6 @@ describe("normalizeAndValidateConfig()", () => { `); expect(diagnostics.renderErrors()).toMatchInlineSnapshot(` "Processing wrangler configuration: - - \\"assets.directory\\" is a required field. - Expected \\"assets.binding\\" to be of type string but got 2." `); }); @@ -1845,27 +1844,6 @@ describe("normalizeAndValidateConfig()", () => { expect(diagnostics.hasErrors()).toBe(false); }); - it("should error if `directory` is an empty string", () => { - const expectedConfig = { - assets: { - directory: "", - }, - }; - - const { config, diagnostics } = normalizeAndValidateConfig( - expectedConfig as unknown as RawConfig, - undefined, - { env: undefined } - ); - - expect(config).toEqual(expect.objectContaining(expectedConfig)); - expect(diagnostics.hasWarnings()).toBeFalsy(); - expect(diagnostics.renderErrors()).toMatchInlineSnapshot(` - "Processing wrangler configuration: - - Expected \\"assets.directory\\" to be a non-empty string." - `); - }); - it("should error on invalid additional fields", () => { const expectedConfig = { assets: { diff --git a/packages/wrangler/src/__tests__/deploy.test.ts b/packages/wrangler/src/__tests__/deploy.test.ts index 9b2717ebc757..84b7c7938bb5 100644 --- a/packages/wrangler/src/__tests__/deploy.test.ts +++ b/packages/wrangler/src/__tests__/deploy.test.ts @@ -4291,7 +4291,7 @@ addEventListener('fetch', event => {});` }); }); - describe("--assets", () => { + describe("assets", () => { it("should use the directory specified in the CLI over wrangler.toml", async () => { const cliAssets = [ { filePath: "cliAsset.txt", content: "Content of file-1" }, @@ -4326,6 +4326,36 @@ addEventListener('fetch', event => {});` }); }); + it("should use the directory specified in the CLI and allow the directory to be missing in the configuration", async () => { + const cliAssets = [ + { filePath: "cliAsset.txt", content: "Content of file-1" }, + ]; + writeAssets(cliAssets, "cli-assets"); + writeWranglerConfig({ + assets: {}, + }); + const bodies: AssetManifest[] = []; + await mockAUSRequest(bodies); + mockSubDomainRequest(); + mockUploadWorkerRequest({ + expectedAssets: { + jwt: "<>", + config: {}, + }, + expectedType: "none", + }); + await runWrangler("deploy --assets cli-assets"); + expect(bodies.length).toBe(1); + expect(bodies[0]).toEqual({ + manifest: { + "/cliAsset.txt": { + hash: "0de3dd5df907418e9730fd2bd747bd5e", + size: 17, + }, + }, + }); + }); + it("should error if config.site and config.assets are used together", async () => { writeWranglerConfig({ main: "./index.js", @@ -4367,6 +4397,28 @@ addEventListener('fetch', event => {});` ); }); + it("should error if the directory path specified by the assets config is undefined", async () => { + writeWranglerConfig({ + assets: {}, + }); + await expect( + runWrangler("deploy") + ).rejects.toThrowErrorMatchingInlineSnapshot( + `[Error: The \`assets\` property in your configuration is missing the required \`directory\` property.]` + ); + }); + + it("should error if the directory path specified by the assets config is an empty string", async () => { + writeWranglerConfig({ + assets: { directory: "" }, + }); + await expect( + runWrangler("deploy") + ).rejects.toThrowErrorMatchingInlineSnapshot( + `[Error: \`The assets directory cannot be an empty string.]` + ); + }); + it("should error if directory specified by config assets does not exist", async () => { writeWranglerConfig({ assets: { directory: "abc" }, diff --git a/packages/wrangler/src/api/integrations/platform/index.ts b/packages/wrangler/src/api/integrations/platform/index.ts index dc4c0b3ba2ce..85f0df177d0e 100644 --- a/packages/wrangler/src/api/integrations/platform/index.ts +++ b/packages/wrangler/src/api/integrations/platform/index.ts @@ -1,5 +1,5 @@ import { kCurrentWorker, Miniflare } from "miniflare"; -import { processAssetsArg } from "../../../assets"; +import { getAssetsOptions } from "../../../assets"; import { readConfig } from "../../../config"; import { DEFAULT_MODULE_RULES } from "../../../deployment-bundle/rules"; import { getBindings } from "../../../dev"; @@ -309,7 +309,7 @@ export function unstable_getMiniflareWorkerOptions( ? getLegacyAssetPaths(config, undefined) : getSiteAssetPaths(config); const sitesOptions = buildSitesOptions({ legacyAssetPaths }); - const processedAssetOptions = processAssetsArg({ assets: undefined }, config); + const processedAssetOptions = getAssetsOptions({ assets: undefined }, config); const assetOptions = processedAssetOptions ? buildAssetOptions({ assets: processedAssetOptions }) : {}; diff --git a/packages/wrangler/src/api/startDevWorker/ConfigController.ts b/packages/wrangler/src/api/startDevWorker/ConfigController.ts index 5b27ae348d9a..5b5239c42513 100644 --- a/packages/wrangler/src/api/startDevWorker/ConfigController.ts +++ b/packages/wrangler/src/api/startDevWorker/ConfigController.ts @@ -9,7 +9,7 @@ import { getScriptName, isLegacyEnv, } from "../.."; -import { processAssetsArg, validateAssetsArgsAndConfig } from "../../assets"; +import { getAssetsOptions, validateAssetsArgsAndConfig } from "../../assets"; import { printBindings, readConfig } from "../../config"; import { getEntry } from "../../deployment-bundle/entry"; import { @@ -248,7 +248,7 @@ async function resolveConfig( const { bindings, unsafe } = await resolveBindings(config, input); - const assetsOptions = processAssetsArg( + const assetsOptions = getAssetsOptions( { assets: input?.assets, script: input.entrypoint, diff --git a/packages/wrangler/src/assets.ts b/packages/wrangler/src/assets.ts index e8fd43bc3a2b..dbd17b16447b 100644 --- a/packages/wrangler/src/assets.ts +++ b/packages/wrangler/src/assets.ts @@ -24,7 +24,6 @@ import { dedent } from "./utils/dedent"; import { createPatternMatcher } from "./utils/filesystem"; import type { StartDevWorkerOptions } from "./api"; import type { Config } from "./config"; -import type { Assets } from "./config/environment"; import type { DeployArgs } from "./deploy"; import type { StartDevOptions } from "./dev"; import type { AssetConfig, RoutingConfig } from "@cloudflare/workers-shared"; @@ -310,12 +309,14 @@ function getAssetsBasePath( : path.resolve(path.dirname(config.configPath ?? "wrangler.toml")); } -export type AssetsOptions = Pick & { +export type AssetsOptions = { + directory: string; + binding?: string; routingConfig: RoutingConfig; assetConfig: AssetConfig; }; -export function processAssetsArg( +export function getAssetsOptions( args: { assets: string | undefined; script?: string }, config: Config ): AssetsOptions | undefined { @@ -325,8 +326,20 @@ export function processAssetsArg( return; } + const { directory, binding } = assets; + + if (directory === undefined) { + throw new UserError( + "The `assets` property in your configuration is missing the required `directory` property." + ); + } + + if (directory === "") { + throw new UserError("`The assets directory cannot be an empty string."); + } + const assetsBasePath = getAssetsBasePath(config, args.assets); - const resolvedAssetsPath = path.resolve(assetsBasePath, assets.directory); + const resolvedAssetsPath = path.resolve(assetsBasePath, directory); if (!existsSync(resolvedAssetsPath)) { const sourceOfTruthMessage = args.assets @@ -339,7 +352,6 @@ export function processAssetsArg( ); } - assets.directory = resolvedAssetsPath; const routingConfig = { has_user_worker: Boolean(args.script || config.main), }; @@ -351,7 +363,8 @@ export function processAssetsArg( }; return { - ...assets, + directory: resolvedAssetsPath, + binding, routingConfig, assetConfig, }; diff --git a/packages/wrangler/src/config/environment.ts b/packages/wrangler/src/config/environment.ts index c70cefe3e6f6..ba8ba4087454 100644 --- a/packages/wrangler/src/config/environment.ts +++ b/packages/wrangler/src/config/environment.ts @@ -940,14 +940,21 @@ export interface UserLimits { export type Assets = { /** Absolute path to assets directory */ - directory: string; + directory?: string; + /** Name of `env` binding property in the User Worker. */ binding?: string; + /** How to handle HTML requests. */ html_handling?: | "auto-trailing-slash" | "force-trailing-slash" | "drop-trailing-slash" | "none"; + /** How to handle requests that do not match an asset. */ not_found_handling?: "single-page-application" | "404-page" | "none"; + /** + * If true, then respond to requests that match an asset with that asset directly. + * If false, route every request to the User Worker, whether or not it matches an asset. + * */ experimental_serve_directly?: boolean; }; diff --git a/packages/wrangler/src/config/validation-helpers.ts b/packages/wrangler/src/config/validation-helpers.ts index f30f106c2601..2229415e00d1 100644 --- a/packages/wrangler/src/config/validation-helpers.ts +++ b/packages/wrangler/src/config/validation-helpers.ts @@ -231,27 +231,6 @@ export const isString: ValidatorFn = (diagnostics, field, value) => { return true; }; -/** - * Validate that the field is a non-empty string. - */ -export const isNonEmptyString: ValidatorFn = ( - diagnostics, - field, - value, - topLevelEnv -) => { - if (!isString(diagnostics, field, value, topLevelEnv)) { - return false; - } - - if ((value as string)?.trim() === "") { - diagnostics.errors.push(`Expected "${field}" to be a non-empty string.`); - return false; - } - - return true; -}; - /** * Validate that the `name` field is compliant with EWC constraints. */ diff --git a/packages/wrangler/src/config/validation.ts b/packages/wrangler/src/config/validation.ts index 5055b629a823..8f62bfa22a39 100644 --- a/packages/wrangler/src/config/validation.ts +++ b/packages/wrangler/src/config/validation.ts @@ -15,7 +15,6 @@ import { inheritableInLegacyEnvironments, isBoolean, isMutuallyExclusiveWith, - isNonEmptyString, isObjectWith, isOneOf, isOptionalProperty, @@ -2122,7 +2121,7 @@ const validateAssetsConfig: ValidatorFn = (diagnostics, field, value) => { // ensure we validate all props before we show the validation errors // this way users have all the necessary info to fix all errors in one go isValid = - validateRequiredProperty( + validateOptionalProperty( diagnostics, field, "directory", @@ -2130,14 +2129,6 @@ const validateAssetsConfig: ValidatorFn = (diagnostics, field, value) => { "string" ) && isValid; - isValid = - isNonEmptyString( - diagnostics, - `${field}.directory`, - (value as Assets).directory, - undefined - ) && isValid; - isValid = validateOptionalProperty( diagnostics, diff --git a/packages/wrangler/src/deploy/index.ts b/packages/wrangler/src/deploy/index.ts index 4bc0c45d0c70..8f724c520960 100644 --- a/packages/wrangler/src/deploy/index.ts +++ b/packages/wrangler/src/deploy/index.ts @@ -1,6 +1,6 @@ import assert from "node:assert"; import path from "node:path"; -import { processAssetsArg, validateAssetsArgsAndConfig } from "../assets"; +import { getAssetsOptions, validateAssetsArgsAndConfig } from "../assets"; import { configFileName, findWranglerConfig, readConfig } from "../config"; import { getEntry } from "../deployment-bundle/entry"; import { UserError } from "../errors"; @@ -301,7 +301,7 @@ async function deployWorker(args: DeployArgs) { validateAssetsArgsAndConfig(args, config); - const assetsOptions = processAssetsArg(args, config); + const assetsOptions = getAssetsOptions(args, config); if (args.latest) { logger.warn( diff --git a/packages/wrangler/src/triggers/index.ts b/packages/wrangler/src/triggers/index.ts index 9f28e0984f2b..1439b411ef4f 100644 --- a/packages/wrangler/src/triggers/index.ts +++ b/packages/wrangler/src/triggers/index.ts @@ -1,4 +1,4 @@ -import { processAssetsArg } from "../assets"; +import { getAssetsOptions } from "../assets"; import { readConfig } from "../config"; import { getScriptName, isLegacyEnv, printWranglerBanner } from "../index"; import * as metrics from "../metrics"; @@ -58,7 +58,7 @@ async function triggersDeployHandler( await printWranglerBanner(); const config = readConfig(undefined, args); - const assetsOptions = processAssetsArg({ assets: undefined }, config); + const assetsOptions = getAssetsOptions({ assets: undefined }, config); await metrics.sendMetricsEvent( "deploy worker triggers", {}, diff --git a/packages/wrangler/src/versions/index.ts b/packages/wrangler/src/versions/index.ts index de4294a02e6c..7734fa2be433 100644 --- a/packages/wrangler/src/versions/index.ts +++ b/packages/wrangler/src/versions/index.ts @@ -1,6 +1,6 @@ import assert from "node:assert"; import path from "node:path"; -import { processAssetsArg, validateAssetsArgsAndConfig } from "../assets"; +import { getAssetsOptions, validateAssetsArgsAndConfig } from "../assets"; import { configFileName, findWranglerConfig, readConfig } from "../config"; import { getEntry } from "../deployment-bundle/entry"; import { UserError } from "../errors"; @@ -242,7 +242,7 @@ async function versionsUploadHandler( config ); - const assetsOptions = processAssetsArg(args, config); + const assetsOptions = getAssetsOptions(args, config); if (args.latest) { logger.warn(