Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: error when asset binding is provided without user worker #6798

Merged
merged 4 commits into from
Sep 23, 2024
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
5 changes: 5 additions & 0 deletions .changeset/silly-birds-shout.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"wrangler": patch
---

fix: error if an asset binding is provided without a Worker script
20 changes: 18 additions & 2 deletions packages/wrangler/src/__tests__/deploy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4345,7 +4345,8 @@ addEventListener('fetch', event => {});`
await expect(
runWrangler("deploy")
).rejects.toThrowErrorMatchingInlineSnapshot(
`[Error: Cannot use Experimental Assets and Workers Sites in the same Worker.]`
dedent`[Error: Cannot use Experimental Assets and Workers Sites in the same Worker.
Please remove either the \`site\` or \`experimental_assets\` field from your configuration file.]`
);
});

Expand All @@ -4360,7 +4361,8 @@ addEventListener('fetch', event => {});`
await expect(
runWrangler("deploy --experimental-assets abc")
).rejects.toThrowErrorMatchingInlineSnapshot(
`[Error: Cannot use Experimental Assets and Workers Sites in the same Worker.]`
dedent`[Error: Cannot use Experimental Assets and Workers Sites in the same Worker.
Please remove either the \`site\` or \`experimental_assets\` field from your configuration file.]`
);
});

Expand All @@ -4385,6 +4387,20 @@ addEventListener('fetch', event => {});`
);
});

it("should error if an ASSET binding is provided without a user Worker", async () => {
writeWranglerToml({
experimental_assets: {
directory: "xyz",
binding: "ASSET",
},
});
await expect(runWrangler("deploy")).rejects
.toThrowErrorMatchingInlineSnapshot(`
[Error: Cannot use Experimental Assets with a binding in an assets-only Worker.
Please remove the asset binding from your configuration file, or provide a Worker script in your configuration file (\`main\`).]
`);
});

it("should be able to upload files with special characters in filepaths", async () => {
// NB windows will disallow these characters in file paths anyway < > : " / \ | ? *
const assets = [
Expand Down
44 changes: 38 additions & 6 deletions packages/wrangler/src/__tests__/dev.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1526,7 +1526,10 @@ describe("wrangler dev", () => {
await expect(
runWrangler("dev")
).rejects.toThrowErrorMatchingInlineSnapshot(
`[Error: Cannot use Experimental Assets and Workers Sites in the same Worker.]`
`
[Error: Cannot use Experimental Assets and Workers Sites in the same Worker.
Please remove either the \`site\` or \`experimental_assets\` field from your configuration file.]
`
);
});

Expand All @@ -1542,7 +1545,10 @@ describe("wrangler dev", () => {
await expect(
runWrangler("dev --experimental-assets assets")
).rejects.toThrowErrorMatchingInlineSnapshot(
`[Error: Cannot use Experimental Assets and Workers Sites in the same Worker.]`
`
[Error: Cannot use Experimental Assets and Workers Sites in the same Worker.
Please remove either the \`site\` or \`experimental_assets\` field from your configuration file.]
`
);
});

Expand All @@ -1563,7 +1569,10 @@ describe("wrangler dev", () => {
await expect(
runWrangler("dev")
).rejects.toThrowErrorMatchingInlineSnapshot(
`[Error: Cannot use Experimental Assets and Legacy Assets in the same Worker.]`
`
[Error: Cannot use Experimental Assets and Legacy Assets in the same Worker.
Please remove either the \`site\` or \`experimental_assets\` field from your configuration file.]
`
);
});

Expand All @@ -1573,7 +1582,10 @@ describe("wrangler dev", () => {
await expect(
runWrangler("dev --experimental-assets assets --legacy-assets assets")
).rejects.toThrowErrorMatchingInlineSnapshot(
`[Error: Cannot use Experimental Assets and Legacy Assets in the same Worker.]`
`
[Error: Cannot use Experimental Assets and Legacy Assets in the same Worker.
Please remove either the \`site\` or \`experimental_assets\` field from your configuration file.]
`
);
});

Expand All @@ -1593,7 +1605,10 @@ describe("wrangler dev", () => {
await expect(
runWrangler("dev --experimental-assets assets")
).rejects.toThrowErrorMatchingInlineSnapshot(
`[Error: Cannot use Experimental Assets and Legacy Assets in the same Worker.]`
`
[Error: Cannot use Experimental Assets and Legacy Assets in the same Worker.
Please remove either the \`site\` or \`experimental_assets\` field from your configuration file.]
`
);
});

Expand All @@ -1609,7 +1624,24 @@ describe("wrangler dev", () => {
await expect(
runWrangler("dev --legacy-assets xyz")
).rejects.toThrowErrorMatchingInlineSnapshot(
`[Error: Cannot use Experimental Assets and Legacy Assets in the same Worker.]`
`
[Error: Cannot use Experimental Assets and Legacy Assets in the same Worker.
Please remove either the \`site\` or \`experimental_assets\` field from your configuration file.]
`
);
});

it("should error if an ASSET binding is provided without a user Worker", async () => {
writeWranglerToml({
experimental_assets: { directory: "assets", binding: "ASSETS" },
});
await expect(
runWrangler("dev")
).rejects.toThrowErrorMatchingInlineSnapshot(
`
[Error: Cannot use Experimental Assets with a binding in an assets-only Worker.
Please remove the asset binding from your configuration file, or provide a Worker script in your configuration file (\`main\`).]
`
);
});

Expand Down
4 changes: 2 additions & 2 deletions packages/wrangler/src/deploy/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { getEntry } from "../deployment-bundle/entry";
import { UserError } from "../errors";
import {
processExperimentalAssetsArg,
verifyMutuallyExclusiveAssetsArgsOrConfig,
validateAssetsArgsAndConfig,
} from "../experimental-assets";
import {
getRules,
Expand Down Expand Up @@ -292,7 +292,7 @@ export async function deployHandler(args: DeployArgs) {
);
}

verifyMutuallyExclusiveAssetsArgsOrConfig(args, config);
validateAssetsArgsAndConfig(args, config);

const experimentalAssetsOptions = processExperimentalAssetsArg(args, config);

Expand Down
4 changes: 2 additions & 2 deletions packages/wrangler/src/dev.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import { startDevServer } from "./dev/start-server";
import { UserError } from "./errors";
import {
processExperimentalAssetsArg,
verifyMutuallyExclusiveAssetsArgsOrConfig,
validateAssetsArgsAndConfig,
} from "./experimental-assets";
import { run } from "./experimental-flags";
import isInteractive from "./is-interactive";
Expand Down Expand Up @@ -621,7 +621,7 @@ export async function startDev(args: StartDevOptions) {
);
}

verifyMutuallyExclusiveAssetsArgsOrConfig(args, config);
validateAssetsArgsAndConfig(args, config);

let experimentalAssetsOptions = processExperimentalAssetsArg(args, config);
if (experimentalAssetsOptions) {
Expand Down
35 changes: 25 additions & 10 deletions packages/wrangler/src/experimental-assets.ts
Original file line number Diff line number Diff line change
Expand Up @@ -371,16 +371,22 @@ export function processExperimentalAssetsArg(
}

/**
* Workers assets cannot be used in combination with a few other select
* Workers features, such as: legacy assets, sites and tail consumers.
*
* This function ensures that if any of these mutually exclusive config
* keys or command args are used together, an appropriate error is thrown.
* Validate assets configuration against the following requirements:
* - assets cannot be used in combination with a few other select
* Workers features, such as: legacy assets, sites and tail consumers
* - an asset binding cannot be used in a Worker that only has assets
* and throw an appropriate error if invalid.
*/
export function verifyMutuallyExclusiveAssetsArgsOrConfig(
export function validateAssetsArgsAndConfig(
args:
| Pick<StartDevOptions, "legacyAssets" | "site" | "experimentalAssets">
| Pick<DeployArgs, "legacyAssets" | "site" | "experimentalAssets">,
| Pick<
StartDevOptions,
"legacyAssets" | "site" | "experimentalAssets" | "script"
>
| Pick<
DeployArgs,
"legacyAssets" | "site" | "experimentalAssets" | "script"
>,
config: Config
) {
/*
Expand All @@ -392,7 +398,8 @@ export function verifyMutuallyExclusiveAssetsArgsOrConfig(
(args.legacyAssets || config.legacy_assets)
) {
throw new UserError(
"Cannot use Experimental Assets and Legacy Assets in the same Worker."
"Cannot use Experimental Assets and Legacy Assets in the same Worker.\n" +
"Please remove either the `site` or `experimental_assets` field from your configuration file."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Please remove either the `site` or `experimental_assets` field from your configuration file."
"Please remove either the `legacy_assets` or `experimental_assets` field from your configuration file."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the wrangler.toml key for sites/legacy assets is sites

);
}

Expand All @@ -401,7 +408,8 @@ export function verifyMutuallyExclusiveAssetsArgsOrConfig(
(args.site || config.site)
) {
throw new UserError(
"Cannot use Experimental Assets and Workers Sites in the same Worker."
"Cannot use Experimental Assets and Workers Sites in the same Worker.\n" +
"Please remove either the `site` or `experimental_assets` field from your configuration file."
);
}

Expand All @@ -413,6 +421,13 @@ export function verifyMutuallyExclusiveAssetsArgsOrConfig(
"Cannot use Experimental Assets and tail consumers in the same Worker. Tail Workers are not yet supported for Workers with assets."
);
}

if (!(args.script || config.main) && config.experimental_assets?.binding) {
throw new UserError(
"Cannot use Experimental Assets with a binding in an assets-only Worker.\n" +
"Please remove the asset binding from your configuration file, or provide a Worker script in your configuration file (`main`)."
);
}
}

const CF_ASSETS_IGNORE_FILENAME = ".assetsignore";
Expand Down
5 changes: 3 additions & 2 deletions packages/wrangler/src/versions/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { getEntry } from "../deployment-bundle/entry";
import { UserError } from "../errors";
import {
processExperimentalAssetsArg,
verifyMutuallyExclusiveAssetsArgsOrConfig,
validateAssetsArgsAndConfig,
} from "../experimental-assets";
import {
getRules,
Expand Down Expand Up @@ -234,14 +234,15 @@ export async function versionsUploadHandler(
);
}

verifyMutuallyExclusiveAssetsArgsOrConfig(
validateAssetsArgsAndConfig(
{
// given that legacyAssets and sites are not supported by
// `wrangler versions upload` pass them as undefined to
// skip the corresponding mutual exclusivity validation
legacyAssets: undefined,
site: undefined,
experimentalAssets: args.experimentalAssets,
script: args.script,
},
config
);
Expand Down
Loading