Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/wrangler-assets-not-a-directory-error.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"wrangler": patch
---

Improve error message when the assets directory path points to a file instead of a directory

Previously, if the path provided as the assets directory (via `--assets` flag or `assets.directory` config) pointed to an existing file rather than a directory, Wrangler would throw an unhelpful `ENOTDIR` system error when trying to read the `_redirects` file inside it. Now Wrangler detects this condition earlier and throws a clear user error.
27 changes: 27 additions & 0 deletions packages/wrangler/src/__tests__/deploy/assets.test.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as fs from "node:fs";
import { writeWranglerConfig } from "@cloudflare/workers-utils/test-helpers";
import { http, HttpResponse } from "msw";
import dedent from "ts-dedent";
Expand Down Expand Up @@ -246,6 +247,32 @@ describe("deploy", () => {
);
});

it("should error if path specified by flag --assets is a file, not a directory", async ({
expect,
}) => {
setIsTTY(false);
fs.writeFileSync("abc", "");
await expect(runWrangler("deploy --assets abc")).rejects
.toThrowErrorMatchingInlineSnapshot(`
[Error: The path specified by the "--assets" command line argument doesn't point to a directory:
<cwd>/abc]
`);
});

it("should error if path specified by config assets.directory is a file, not a directory", async ({
expect,
}) => {
writeWranglerConfig({
assets: { directory: "abc" },
});
fs.writeFileSync("abc", "");
await expect(runWrangler("deploy")).rejects
.toThrowErrorMatchingInlineSnapshot(`
[Error: The path specified by the "assets.directory" field in your configuration file doesn't point to a directory:
<cwd>/abc]
`);
});

it("should error if an ASSET binding is provided without a user Worker", async ({
expect,
}) => {
Expand Down
29 changes: 29 additions & 0 deletions packages/wrangler/src/__tests__/dev.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2274,6 +2274,35 @@ describe.sequential("wrangler dev", () => {
)
);
});

it("should error with a clear error message if the path specified by '--assets' command line argument is a file, not a directory", async () => {
writeWranglerConfig({
main: "./index.js",
});
fs.writeFileSync("index.js", `export default {};`);
fs.writeFileSync("abc", "");
await expect(runWrangler("dev --assets abc")).rejects
.toThrowErrorMatchingInlineSnapshot(`
[Error: The path specified by the "--assets" command line argument doesn't point to a directory:
<cwd>/abc]
`);
});

it("should error with a clear error message if the path specified by '[assets]' configuration key is a file, not a directory", async () => {
writeWranglerConfig({
main: "./index.js",
assets: {
directory: "abc",
},
});
fs.writeFileSync("index.js", `export default {};`);
fs.writeFileSync("abc", "");
await expect(runWrangler("dev")).rejects
.toThrowErrorMatchingInlineSnapshot(`
[Error: The path specified by the "assets.directory" field in your configuration file doesn't point to a directory:
<cwd>/abc]
`);
});
});

describe("service bindings", () => {
Expand Down
45 changes: 35 additions & 10 deletions packages/wrangler/src/assets.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import assert from "node:assert";
import { existsSync } from "node:fs";
import { statSync } from "node:fs";
import { readdir, readFile, stat } from "node:fs/promises";
import * as path from "node:path";
import { parseStaticRouting } from "@cloudflare/workers-shared/utils/configuration/parseStaticRouting";
Expand Down Expand Up @@ -108,7 +108,9 @@ export const syncAssets = async (
// 3. fill buckets and upload assets
const numberFilesToUpload = initializeAssetsResponse.buckets.flat().length;
logger.info(
`🌀 Found ${numberFilesToUpload} new or modified static asset${numberFilesToUpload > 1 ? "s" : ""} to upload. Proceeding with upload...`
`🌀 Found ${numberFilesToUpload} new or modified static asset${
numberFilesToUpload > 1 ? "s" : ""
} to upload. Proceeding with upload...`
);

// Create the buckets outside of doUpload so we can retry without losing track of potential duplicate files
Expand Down Expand Up @@ -216,7 +218,9 @@ export const syncAssets = async (
} else if (isJwtExpired(initializeAssetsResponse.jwt)) {
throw new FatalError(
`Upload took too long.\n` +
`Asset upload took too long on bucket ${bucketIndex + 1}/${initializeAssetsResponse.buckets.length}. Please try again.\n` +
`Asset upload took too long on bucket ${bucketIndex + 1}/${
initializeAssetsResponse.buckets.length
}. Please try again.\n` +
`Assets already uploaded have been saved, so the next attempt will automatically resume from this point.`,
undefined,
{ telemetryMessage: "Asset upload took too long" }
Expand Down Expand Up @@ -258,7 +262,9 @@ export const syncAssets = async (
const skippedMessage = skipped > 0 ? `(${skipped} already uploaded) ` : "";

logger.log(
`✨ Success! Uploaded ${numberFilesToUpload} file${numberFilesToUpload > 1 ? "s" : ""} ${skippedMessage}${formatTime(uploadMs)}\n`
`✨ Success! Uploaded ${numberFilesToUpload} file${
numberFilesToUpload > 1 ? "s" : ""
} ${skippedMessage}${formatTime(uploadMs)}\n`
);

return completionJwt;
Expand Down Expand Up @@ -348,7 +354,9 @@ function logAssetsUploadStatus(
uploadedAssetFiles: string[]
) {
logger.info(
`Uploaded ${uploadedAssetsCount} of ${numberFilesToUpload} asset${numberFilesToUpload === 1 ? "" : "s"}`
`Uploaded ${uploadedAssetsCount} of ${numberFilesToUpload} asset${
numberFilesToUpload === 1 ? "" : "s"
}`
);
uploadedAssetFiles.forEach((file) => logger.debug(`✨ ${file}`));
}
Expand All @@ -360,7 +368,9 @@ function logAssetsUploadStatus(
*/
function logReadFilesFromDirectory(directory: string, assetFiles: string[]) {
logger.info(
`✨ Read ${assetFiles.length} file${assetFiles.length === 1 ? "" : "s"} from the assets directory ${directory}`
`✨ Read ${assetFiles.length} file${
assetFiles.length === 1 ? "" : "s"
} from the assets directory ${directory}`
);
assetFiles.forEach((file) => logger.debug(`/${file}`));
}
Expand Down Expand Up @@ -390,6 +400,8 @@ export type AssetsOptions = {

export class NonExistentAssetsDirError extends UserError {}

export class NonDirectoryAssetsDirError extends UserError {}

export function getAssetsOptions(
args: { assets: string | undefined; script?: string },
config: Config,
Expand Down Expand Up @@ -421,11 +433,13 @@ export function getAssetsOptions(
const assetsBasePath = getAssetsBasePath(config, args.assets);
const directory = path.resolve(assetsBasePath, assets.directory);

if (!existsSync(directory)) {
const sourceOfTruthMessage = args.assets
? '"--assets" command line argument'
: '"assets.directory" field in your configuration file';
const directoryStat = statSync(directory, { throwIfNoEntry: false });

const sourceOfTruthMessage = args.assets
? '"--assets" command line argument'
: '"assets.directory" field in your configuration file';

if (!directoryStat) {
throw new NonExistentAssetsDirError(
`The directory specified by the ${sourceOfTruthMessage} does not exist:\n` +
`${directory}`,
Expand All @@ -436,6 +450,17 @@ export function getAssetsOptions(
);
}

if (!directoryStat.isDirectory()) {
throw new NonDirectoryAssetsDirError(
`The path specified by the ${sourceOfTruthMessage} doesn't point to a directory:\n` +
`${directory}`,

{
telemetryMessage: `The assets directory path specified is not a directory`,
}
);
}

const routerConfig: RouterConfig = {
has_user_worker: Boolean(args.script || config.main),
};
Expand Down
Loading