Add --show-diff flag to wrangler triggers deploy#14114
Conversation
🦋 Changeset detectedLatest commit: 58c0123 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
|
✅ All changesets look good |
![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 PR adds a --show-diff flag to wrangler triggers deploy that fetches the remote trigger config and diffs it against the local config, supporting both standalone and --dry-run modes.
Issues
-
Deployment timing inflated by
--show-diffnetwork calls — When--show-diffis used without--dry-run, thefetchRemoteTriggerscall (6 API requests viafetchWorkerConfig) runs betweenstart = Date.now()anduploadMs = Date.now() - start, inflating the reported deployment time. -
fetchWorkerConfigmakes 6 API requests but only 4 are used —fetchRemoteTriggerscallsfetchWorkerConfigwhich fetches bindings and service environment metadata in addition to the 4 things actually used (routes, custom domains, subdomain status, cron triggers). This adds unnecessary latency and API load every time--show-diffis used. Consider either adding a lighter-weight fetch function or documenting why the reuse is acceptable.
| return; | ||
| } | ||
|
|
||
| const uploadMs = Date.now() - start; |
There was a problem hiding this comment.
uploadMs now includes the time spent in fetchRemoteTriggers (which makes 6 API calls via fetchWorkerConfig), inflating the reported deployment timing when --show-diff is used.
Move the start capture below the diff fetch, or capture the time just before the actual deployments begin:
| const uploadMs = Date.now() - start; | |
| const deployStart = Date.now(); |
And then compute deployMs later as Date.now() - deployStart - uploadMs accordingly — or simpler, just move the snapshot fetch before start.
| if (props.showDiff) { | ||
| remoteTriggersSnapshot = await fetchRemoteTriggers( | ||
| accountId, | ||
| scriptName, | ||
| envName | ||
| ); | ||
| } |
There was a problem hiding this comment.
fetchWorkerConfig makes 6 parallel API requests (bindings, routes, custom domains, subdomain, service env metadata, cron triggers), but fetchRemoteTriggers only uses 4 of those results. The wasted bindings + service-env-metadata fetches add latency and unnecessary API traffic.
Consider adding a targeted fetch function (e.g. fetchWorkerTriggerConfig) that only requests the 4 endpoints needed, or at minimum add a comment explaining the trade-off.
|
I've posted my review on PR #14114. Here's a summary of the two issues I flagged:
|
create-cloudflare
@cloudflare/deploy-helpers
@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
@cloudflare/wrangler-bundler
commit: |
2 participants
Fixes #[insert GH or internal issue link(s)].
When
--show-diffis passed, the command fetches the currently deployed trigger configuration from the remote worker and displays a colored diff against the local configuration. This covers routes, custom domains, cron schedules, and theworkers_dev/preview_urlssubdomain settings.The flag also works with
--dry-runto preview what trigger changes would be applied without actually deploying.A picture of a cute animal (not mandatory, but encouraged)