Prompt for Worker name interactively when not provided during wrangler deploy#13699
Prompt for Worker name interactively when not provided during wrangler deploy#13699dario-piotrowicz wants to merge 1 commit intomainfrom
wrangler deploy#13699Conversation
🦋 Changeset detectedLatest commit: d185565 The changes in this PR will be included in the next version bump. 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 |
|
✅ All changesets look good Review SummaryFile reviewed: Validation Results
Description QualityThe changeset provides a comprehensive description that includes:
|
|
I see the inconsistency: lines 23 and 30 say "Worker names" but lines 38 and 45 say "Project names". However, this was pre-existing in the old code. Not something introduced by this PR. Now let me look at one potentially real issue — the Wait, let me re-examine: the regex is This is a bug — but it's pre-existing from the old code. The space in the regex was there before this PR. However, this PR is now exposing it to a new codepath (the interactive prompt validation), where users can type spaces and have them accepted. In the old autoconfig flow, the name was derived from directory/project names which are less likely to contain spaces, and went through Actually, let me think more carefully. Is a Worker name with a space actually invalid per the Cloudflare API? The docs say "lowercase alphanumeric and dashes". If so, the space in the regex is a bug. But since this PR is just moving the code and the regex is unchanged, I'd note it but not block on it. Let me now focus on what's actually new in this PR and check for real issues:
Let me check one more thing — the Consider
This would return a name ending with dashes. But again, this is pre-existing. Let me focus on the one actionable issue I see that's introduced by this PR: the Actually wait — I need to be more precise. The space is allowed by the regex. But would Cloudflare's API reject it? Let me check the Cloudflare docs reference in the comment. The regex But this is pre-existing code. The PR just moved it. I shouldn't flag pre-existing issues unless they're made worse. Is there anything new the PR introduces that's problematic? One thing: the But if a user enters Let me check if there are any actual issues I should flag. Let me re-read the full diff one more time carefully... Looking at const rawDefault = !genericNames.includes(scriptBasename.toLowerCase())
? scriptBasename
: path.basename(process.cwd());The But I think this PR is clean. The code change is logical, the tests cover the key scenarios, the refactoring is straightforward. The pre-existing issues (space in regex, inconsistent error messages) are not introduced by this PR. LGTM |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
workers-devprod
requested review from
a team and
ascorbic
and removed request for
a team
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
|
||
| Prompt for Worker name interactively when not provided during `wrangler deploy` | ||
|
|
||
| When deploying without a `name` in your Wrangler config or `--name` CLI argument, `wrangler deploy` now interactively prompts you to choose a Worker name instead of failing. The default suggestion is derived from the entry-point filename (e.g. `my-api.ts` → `my-api`), falling back to the current directory name when the entry-point has a generic name like `index.js` or `worker.ts`. Input is validated against Cloudflare's Worker naming rules (lowercase alphanumeric and dashes, max 63 characters). In non-interactive or CI environments, the existing error message is still shown. |
There was a problem hiding this comment.
I wonder if we should simplify the default suggestion to always use the current directory name.
Deriving it from the entrypoint filename could be confusing because it's hard to define what counts as a generic name here. Some users might reasonably expect api.js or server.ts to be considered generic too.
The current directory name feels simpler and more predictable as a default. 🤔
There was a problem hiding this comment.
mh... but I was wondering... what if the user has a bunch of different worker entrypoint files in the same directory? 🤔
There was a problem hiding this comment.
Yeah, that could happen. I'm not sure it's something we need to optimize the default for though, as I would still expect most people uses a monorepo setup if they have multiple workers.
But I don't feel strongly here, so happy to leave it up to you. :)
There was a problem hiding this comment.
How about if you run:
cd my-project
wrangler deploy src/blah-worker.js
It could say:
> Please choose a worker name:
- `my-project` (current working directory name)
- `src` (entry-point containing directory name)
- `blah-worker` (entry-point file name)
- enter your preferred name
There was a problem hiding this comment.
then the user could just select which they wanted to use
There was a problem hiding this comment.
The current solution uses the file or directory name as the default value, but the user can still type and use whatever name they prefer, I feel like giving them a list to chose from might be going a bit too far 🤔
| { telemetryMessage: true } | ||
| ); | ||
| } else { | ||
| name = await promptForWorkerName(entry.file); |
There was a problem hiding this comment.
Should we create / update the wrangler config with the name they selected? Just so they don't run into this prompt again next time and accidentally uses a different name.
There was a problem hiding this comment.
I think that if there is a config file, it should error.
There was a problem hiding this comment.
mh... yes... I think it does error since wrangler should fail at the config validation and not reach this point... I'll have a look to make sure that's the case 🤔
| const defaultName = toValidWorkerName(rawDefault); | ||
|
|
||
| const name = await prompt( | ||
| "What would you like your Worker to be named? (Note: You can also set the name via the `--name` CLI argument or `name` in your Wrangler config file)", |
There was a problem hiding this comment.
Do we need the note in the prompt text? It makes the question quite long, and at this point the user mostly needs to answer what name they want.
There was a problem hiding this comment.
I agree that it makes the question too long! 😓
But it is still useful info... I guess we could present it after the question has been answered, would that work for you?
There was a problem hiding this comment.
I am not sure if I follow. Why would the user cancel the prompt and use the flag instead? 🤔
There was a problem hiding this comment.
mh.... no mostly, regardless on whether the user accepts or cancels, it's good to tell them that they can set it in their config or with the cli flag, so that next time they can use that and not get this prompt again
There was a problem hiding this comment.
I see. I guess that's why I was suggesting if we can generate a wrangler config with the name they selected. If we are not doing that, this makes sense :)
There was a problem hiding this comment.
I see. I guess that's why I was suggesting if we can generate a wrangler config with the name they selected.
The idea here is not to try to force users to necessarily have a config file, basically this is mainly trying to satisfy users that just want to run wrangler deploy against a single plain file without needing any extra configuration or flags
| export function checkWorkerNameValidity( | ||
| input: string | ||
| ): { valid: false; cause: string } | { valid: true } { | ||
| if (!input) { | ||
| return { | ||
| valid: false, | ||
| cause: "Worker names cannot be empty.", | ||
| }; | ||
| } | ||
|
|
||
| if (input.match(invalidWorkerNameStartEndRegex)) { | ||
| return { | ||
| valid: false, | ||
| cause: "Worker names cannot start or end with a dash.", | ||
| }; | ||
| } | ||
|
|
||
| if (input.match(invalidWorkerNameCharsRegex)) { | ||
| return { | ||
| valid: false, | ||
| cause: | ||
| "Project names must only contain lowercase characters, numbers, and dashes.", | ||
| }; | ||
| } | ||
|
|
||
| if (input.length > workerNameLengthLimit) { | ||
| return { | ||
| valid: false, | ||
| cause: "Project names must be less than 63 characters.", | ||
| }; | ||
| } | ||
|
|
||
| return { valid: true }; | ||
| } |
There was a problem hiding this comment.
Would it be possible to reuse or share logic with the existing name validation in packages/workers-utils/src/config/validation-helpers.ts? I'd prefer to keep the prompt validation pretty minimal and avoid adding another slightly different version of Worker name validation.
|
copying over a comment from #13701 - we should consolidate this with existing logic in |
|
converting to draft as I might just wait to merge #13701 first 🙂 |
5 participants
When deploying without a
namein your Wrangler config or--nameCLI argument,wrangler deploynow interactively prompts you to choose a Worker name instead of failing. The default suggestion is derived from the entry-point filename (e.g.my-api.ts→my-api), falling back to the current directory name when the entry-point has a generic name likeindex.jsorworker.ts. Input is validated against Cloudflare's Worker naming rules (lowercase alphanumeric and dashes, max 63 characters). In non-interactive or CI environments, the existing error message is still shown.A picture of a cute animal (not mandatory, but encouraged)