Skip to content

refactor commands handling #12237

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.

Sign up for GitHub

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

Jump to bottom
Merged
vicb merged 13 commits into from
Jan 29, 2026
Merged

refactor commands handling #12237

vicb merged 13 commits into from
Jan 29, 2026

Conversation

@vicb
Copy link
Contributor

@vicb vicb commented Jan 29, 2026 •
edited by devin-ai-integration bot
Loading

Fixes #12029

3 commits in this PR:

Note about the third commit: we should probably only be sending ms but I don't want this PR to change the current behavior. I'll follow up with another PR.

/cc @MattieTK

  • Tests
    • Tests included/updated
    • Automated tests not possible - manual testing has been completed as follows:
    • Additional testing not necessary because:
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because: not change

A picture of a cute animal (not mandatory, but encouraged)

Open with Devin

@vicb vicb requested a review from a team as a code owner January 29, 2026 09:26
@vicb vicb added wrangler Relating to the Wrangler CLI tool no-changeset-required tech debt labels Jan 29, 2026
@changeset-bot
Copy link

changeset-bot bot commented Jan 29, 2026 •
edited
Loading

⚠️ No Changeset found

Latest commit: 8b128f3

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-project-automation github-project-automation bot moved this to Untriaged in workers-sdk Jan 29, 2026
@vicb vicb changed the title Vicb/cmd behavior refactor commands handling Jan 29, 2026
devin-ai-integration[bot]
devin-ai-integration bot reviewed Jan 29, 2026
View reviewed changes
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

Open in Devin Review

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jan 29, 2026 •
edited
Loading

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@12237

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@12237

miniflare

npm i https://pkg.pr.new/miniflare@12237

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@12237

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@12237

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@12237

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@12237

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@12237

@cloudflare/workers-utils

npm i https://pkg.pr.new/@cloudflare/workers-utils@12237

wrangler

npm i https://pkg.pr.new/wrangler@12237

commit: 8b128f3

devin-ai-integration[bot]
devin-ai-integration bot reviewed Jan 29, 2026
View reviewed changes
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 new potential issue.

View issue and 5 additional flags in Devin Review.

Open in Devin Review

devin-ai-integration[bot]
devin-ai-integration bot reviewed Jan 29, 2026
View reviewed changes
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 new potential issue.

View issue and 7 additional flags in Devin Review.

Open in Devin Review

devin-ai-integration[bot]
devin-ai-integration bot reviewed Jan 29, 2026
View reviewed changes
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 new potential issue.

View issue and 10 additional flags in Devin Review.

Open in Devin Review

devin-ai-integration[bot]
devin-ai-integration bot reviewed Jan 29, 2026
View reviewed changes
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 new potential issue.

View issue and 9 additional flags in Devin Review.

Open in Devin Review

vicb
vicb commented Jan 29, 2026
View reviewed changes
vicb
vicb commented Jan 29, 2026
View reviewed changes
petebacondarwin
petebacondarwin approved these changes Jan 29, 2026
View reviewed changes
Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

LGTM with a few nits

packages/wrangler/src/metrics/metrics-dispatcher.ts
properties.sanitizedCommand === "docs"
) {
if (cmdBehaviour?.printMetricsBanner === true) {
// printMetricsBanner can throw
Copy link
Contributor

@petebacondarwin petebacondarwin Jan 29, 2026

Choose a reason for hiding this comment

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

Is this comment needed? There are plenty of function calls throughout the codebase that can throw and we don't call that out.
If this comment is particularly relevant then we should say why it could throw and why we need to know that here.

Copy link
Contributor Author

@vicb vicb Jan 29, 2026

Choose a reason for hiding this comment

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

I think it is.

My initial version move printMetricsBanner outside of the try/catch because why would printing throw?

we should say why it could throw

Good point and why I added JSDoc on that function ;)

Copy link
Contributor Author

@vicb vicb Jan 29, 2026

Choose a reason for hiding this comment

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

Also #12237 (comment)

@github-project-automation github-project-automation bot moved this from Untriaged to Approved in workers-sdk Jan 29, 2026
@vicb vicb force-pushed the vicb/cmd-behavior branch from 1265ddb to ee6e114 Compare January 29, 2026 17:23
@vicb vicb merged commit 8966d71 into main Jan 29, 2026
39 of 40 checks passed
@vicb vicb deleted the vicb/cmd-behavior branch January 29, 2026 18:34
@github-project-automation github-project-automation bot moved this from Approved to Done in workers-sdk Jan 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@petebacondarwin petebacondarwin petebacondarwin approved these changes

@devin-ai-integration devin-ai-integration[bot] devin-ai-integration[bot] left review comments

Assignees

No one assigned

Labels

no-changeset-required tech debt wrangler Relating to the Wrangler CLI tool

Projects

workers-sdk
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

refactor: getMetricsDispatcher#sendCommandEvent should not rely on command names

2 participants

@vicb @petebacondarwin