Refactor#13845
Conversation
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This refactoring extracts shared deploy/versions-upload logic into shared.ts, moves autoconfig and open-next delegation into autoconfig.ts, and inlines the core deploy logic from deploy() into deployWorker() inside index.ts.
Issues found (ranked by severity)
-
Duplicate
describe("legacy")block in test file — lines 1132-1141 are an incomplete copy of the block that starts at line 1142. This will cause a parse/syntax error or malformed test structure. -
D1 binding special error handler removed — The old
deploy()function had a special case that caught API error 10021 with the specific D1 binding message and re-threw a user-friendly error. That handler was deleted from the newdeployWorker()inindex.ts(compare with the old code around thecatchblock). The source-mapping logic was kept, but the D1 guard was dropped. -
telemetryMessage: trueused for missing worker name — Perpackages/wrangler/AGENTS.md,telemetryMessage: truesends the user-facing message as the telemetry label and is discouraged when a stable static label is available. The old deploy and versions-upload paths had distinct stable labels (deploy command missing worker name/versions upload missing worker name).
| describe("legacy", () => { | ||
| it("uses the script name when no environment is specified", async ({ | ||
| expect, | ||
| }) => { | ||
| writeWranglerConfig(); | ||
| writeWorkerSource(); | ||
| mockSubDomainRequest(); | ||
| mockUploadWorkerRequest({ | ||
| useServiceEnvironments: false, | ||
| }); |
There was a problem hiding this comment.
This block (lines 1132-1141) is an incomplete duplicate of the describe("legacy") block that immediately follows it at line 1142. It opens a describe and it callback, sets up mocks, but never closes any braces or calls runWrangler. This will either cause a syntax error or produce a broken nested test structure.
| describe("legacy", () => { | |
| it("uses the script name when no environment is specified", async ({ | |
| expect, | |
| }) => { | |
| writeWranglerConfig(); | |
| writeWorkerSource(); | |
| mockSubDomainRequest(); | |
| mockUploadWorkerRequest({ | |
| useServiceEnvironments: false, | |
| }); |
| err.notes[0].text, | ||
| retrieveSourceMap | ||
| ); | ||
| } |
There was a problem hiding this comment.
The old deploy() function had a D1-specific error handler here that caught API error 10021 with the message "binding DB of type d1 must have a valid \id` specified [code: 10021]"and re-threw a user-friendlyUserErrorpointing towrangler d1 list`. That handler was removed in this refactor — was that intentional? If not, it should be re-added before the source-mapping logic:
| } | |
| if ( | |
| err.notes[0].text === | |
| "binding DB of type d1 must have a valid `id` specified [code: 10021]" | |
| ) { | |
| throw new UserError( | |
| "You must use a real database in the database_id configuration. You can find your databases using 'wrangler d1 list', or read how to develop locally with D1 here: https://developers.cloudflare.com/d1/configuration/local-development", | |
| { telemetryMessage: "deploy d1 database binding invalid id" } | |
| ); | |
| } |
| if (!name) { | ||
| throw new UserError( | ||
| 'You need to provide a name of your worker. Either pass it as a cli arg with `--name <name>` or in your config file as `name = "<name>"`', | ||
| { telemetryMessage: true } |
There was a problem hiding this comment.
Per packages/wrangler/AGENTS.md: "telemetryMessage: true is usually less useful because user-facing copy is harder to group and parse than a stable telemetry label." The old code used distinct labels: "deploy command missing worker name" and "versions upload missing worker name". Since resolveSharedConfig already receives a command parameter, you could use it to preserve the distinction:
| { telemetryMessage: true } | |
| { telemetryMessage: `${command} missing worker name` } |
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: |
|
Review posted successfully on PR #13845. Here's a summary of what I found:
|
2 participants
Fixes #[insert GH or internal issue link(s)].
Describe your change...
A picture of a cute animal (not mandatory, but encouraged)