Skip to content

Commit

Permalink
fix: allow the asset directory to be omitted in Wrangler config for c…
Browse files Browse the repository at this point in the history
…ommands that don't need it (#7426)
  • Loading branch information
petebacondarwin authored Dec 4, 2024
1 parent a3d5aad commit b40d0ab
Show file tree
Hide file tree
Showing 12 changed files with 96 additions and 71 deletions.
5 changes: 5 additions & 0 deletions .changeset/forty-gifts-grab.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"wrangler": patch
---

fix: allow the asset directory to be omitted in Wrangler config for commands that don't need it
22 changes: 0 additions & 22 deletions packages/wrangler/src/__tests__/configuration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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."
`);
});
Expand Down Expand Up @@ -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: {
Expand Down
54 changes: 53 additions & 1 deletion packages/wrangler/src/__tests__/deploy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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" },
Expand Down Expand Up @@ -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: "<<aus-completion-token>>",
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",
Expand Down Expand Up @@ -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" },
Expand Down
4 changes: 2 additions & 2 deletions packages/wrangler/src/api/integrations/platform/index.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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 })
: {};
Expand Down
4 changes: 2 additions & 2 deletions packages/wrangler/src/api/startDevWorker/ConfigController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down
25 changes: 19 additions & 6 deletions packages/wrangler/src/assets.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -310,12 +309,14 @@ function getAssetsBasePath(
: path.resolve(path.dirname(config.configPath ?? "wrangler.toml"));
}

export type AssetsOptions = Pick<Assets, "directory" | "binding"> & {
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 {
Expand All @@ -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
Expand All @@ -339,7 +352,6 @@ export function processAssetsArg(
);
}

assets.directory = resolvedAssetsPath;
const routingConfig = {
has_user_worker: Boolean(args.script || config.main),
};
Expand All @@ -351,7 +363,8 @@ export function processAssetsArg(
};

return {
...assets,
directory: resolvedAssetsPath,
binding,
routingConfig,
assetConfig,
};
Expand Down
9 changes: 8 additions & 1 deletion packages/wrangler/src/config/environment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};

Expand Down
21 changes: 0 additions & 21 deletions packages/wrangler/src/config/validation-helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down
11 changes: 1 addition & 10 deletions packages/wrangler/src/config/validation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import {
inheritableInLegacyEnvironments,
isBoolean,
isMutuallyExclusiveWith,
isNonEmptyString,
isObjectWith,
isOneOf,
isOptionalProperty,
Expand Down Expand Up @@ -2122,22 +2121,14 @@ 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",
(value as Assets).directory,
"string"
) && isValid;

isValid =
isNonEmptyString(
diagnostics,
`${field}.directory`,
(value as Assets).directory,
undefined
) && isValid;

isValid =
validateOptionalProperty(
diagnostics,
Expand Down
4 changes: 2 additions & 2 deletions packages/wrangler/src/deploy/index.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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(
Expand Down
4 changes: 2 additions & 2 deletions packages/wrangler/src/triggers/index.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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",
{},
Expand Down
4 changes: 2 additions & 2 deletions packages/wrangler/src/versions/index.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -242,7 +242,7 @@ async function versionsUploadHandler(
config
);

const assetsOptions = processAssetsArg(args, config);
const assetsOptions = getAssetsOptions(args, config);

if (args.latest) {
logger.warn(
Expand Down

0 comments on commit b40d0ab

Please sign in to comment.