feat: interactively handle wonky asset deploy commands #10016
Conversation
🦋 Changeset detectedLatest commit: 8c6c672 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
emily-shen
force-pushed
the
emily/assets-interactive-deploy
branch
from
2984c23 to
164b6fd
Compare
| try { | ||
| const stats = statSync(args.script); | ||
| if (stats.isDirectory()) { | ||
| args = await handleMaybeAssetsDeployment(args); |
There was a problem hiding this comment.
Consider moving the dir check inside handleMaybeAssetsDeployment ?
Maybe comment on handleMaybeAssetsDeployment what the req are (i.e. directory)
There was a problem hiding this comment.
I think the dir check here is correct but the signature on handleMaybeAssetsDeployment() should be something like (assetDirectory: string, args: DeployArgs) to make it clear that this function expects a directory of some kind.
| const projectName = await prompt("What do you want to name your project?", { | ||
| defaultValue: process.cwd().split(path.sep).pop(), | ||
| }); | ||
| args.name = projectName; |
There was a problem hiding this comment.
Will it not get validated later in the normal deploy validation?
There was a problem hiding this comment.
i'll validate that the default we suggest is valid, at least.
| try { | ||
| const stats = statSync(args.script); | ||
| if (stats.isDirectory()) { | ||
| args = await handleMaybeAssetsDeployment(args); |
There was a problem hiding this comment.
I think the dir check here is correct but the signature on handleMaybeAssetsDeployment() should be something like (assetDirectory: string, args: DeployArgs) to make it clear that this function expects a directory of some kind.
| const projectName = await prompt("What do you want to name your project?", { | ||
| defaultValue: process.cwd().split(path.sep).pop(), | ||
| }); | ||
| args.name = projectName; |
There was a problem hiding this comment.
Will it not get validated later in the normal deploy validation?
| ); | ||
|
|
||
| if (writeConfig) { | ||
| const configPath = path.join(process.cwd(), "wrangler.json"); |
There was a problem hiding this comment.
We definitely encourage JSONC.
Moreover, we could actually create a JSONC file with comments in if we felt inclined.
| const configPath = path.join(process.cwd(), "wrangler.json"); | |
| const configPath = path.join(process.cwd(), "wrangler.jsonc"); |
There was a problem hiding this comment.
We use this in C3 - https://www.npmjs.com/package/comment-json
There was a problem hiding this comment.
updated to use jsonc. i can't really think of a useful comment to add though, so i might just leave it as is. since its assets only, linking to the full wrangler config docs might just cause more confusion.
emily-shen
force-pushed
the
emily/assets-interactive-deploy
branch
from
164b6fd to
8c6c672
Compare
Interactively handle
wrangler deploys that are probably assets-only, where there is no config file found and flags are incorrect or missing.For example:
npx wrangler deploy ./publicwill now ask if you meant to deploy an folder of assets only, ask for a name, set the compat date and then ask to write your choices out towrangler.jsonfor subsequent deployments.npx wrangler deploy --assets=./publicwill now ask for a name, set the compat date and then ask to write your choices out towrangler.jsonfor subsequent deployments.In non-interactive contexts, we will error as we currently do.
Out of scope (for this PR) are worker or worker and asset deploys.