defineCommand util #6526

RamIdeas · 2024-08-19T15:00:53Z

What this PR solves / how to test

This PR introduces a thin abstraction around yargs to improve wrangler's CLI command registration with 3 utilities:

defineCommand: define an absolute command (wrangler [..namespace] subcommand) – satisfying the typescript compiler will ensure all necessary info is provided (description, status, owner, args, handler, etc...)
defineNamespace: define an absolute command namespace (wrangler <..namespace>) – satisfying the typescript compiler will ensure all necessary info is provided (description, status, owner, etc...)
defineAlias: define an alias from any absolute command to any other absolute command. Optionally, override metadata (eg description, status, deprecated, hidden, etc...).

Implementation details:
These 3 core utils are in src/core/define-commands. They simply push the definition into a flat array of all definitions.

In src/core/register-commands, a tree is built from the flat list and is validated, and exposes utils to register commands with yargs: register.registerAllCommands, register.registerNamespace (the latter of which is needed to register commands in the same order as the existing implementation).

Before registering the commands, the definitions are wrapped with src/core/wrap-commands to provide centralised behaviour and transforms the definition into something more friendly to pass to yargs.

Addressed:

current contrib experience starts with copy/paste
leading to inconsistencies
required knowledge of yargs (not obvious)

Wins:

ensure subtleties of command generation are handled consistently, centrally
- subhelp by default
- deprecated messaging handled
- status (experimental, alpha, beta, etc) messaging handled
- easier definition of args sharing arg definitions (as plain objects)
less random yargs builder functions importing them across multiple files
- removed alias complexity
less complex yargs generic types
less naming fatigue export/import chains
- move code where it makes sense, not where yargs makes you
declarative interface with types such that any missing info is caught by type checking
- PR reviews can focus on implementation and not usage of yargs
definitions separated from the registration
- module graph simplified
- enabling test improvements in future

Author has addressed the following

Tests
- TODO (before merge)
- Included
- Not necessary because:
E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
- I don't know
- Required / Maybe required
- Not required because:
Changeset (Changeset guidelines)
- TODO (before merge)
- Included
- Not necessary because: no user facing changes
Public documentation
- TODO (before merge)
- Cloudflare docs PR(s):
- Not necessary because: internal change

changeset-bot · 2024-08-19T15:00:57Z

⚠️ No Changeset found

Latest commit: ca3f113

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.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

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

packages/wrangler/src/versions/deploy.ts

github-actions · 2024-08-21T19:50:24Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11123028091/npm-package-wrangler-6526

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6526/npm-package-wrangler-6526

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11123028091/npm-package-wrangler-6526 dev path/to/script.js

Additional artifacts:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11123028091/npm-package-create-cloudflare-6526 --no-auto-update

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11123028091/npm-package-cloudflare-kv-asset-handler-6526

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11123028091/npm-package-miniflare-6526

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11123028091/npm-package-cloudflare-pages-shared-6526

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11123028091/npm-package-cloudflare-vitest-pool-workers-6526

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11123028091/npm-package-cloudflare-workers-editor-shared-6526

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11123028091/npm-package-cloudflare-workers-shared-6526

Note that these links will no longer work once the GitHub Actions artifact expires.

[email protected] includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20240925.0
`workerd`	1.20240925.0	1.20240925.0
`workerd --version`	1.20240925.0	2024-09-25

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

packages/wrangler/CONTRIBUTING.md

packages/wrangler/src/core/teams.d.ts

packages/wrangler/src/__tests__/core/command-registration.test.ts

packages/wrangler/CONTRIBUTING.md

packages/wrangler/src/core/define-command.ts

packages/wrangler/src/core/register-commands.ts

packages/wrangler/src/core/wrap-command.ts

penalosa · 2024-09-09T20:12:08Z

packages/wrangler/src/core/wrap-command.ts

+
+ statusMessage =
+ def.metadata.statusMessage ??
+ `🚧 \`${def.command}\` is a ${def.metadata.status} command. Please report any issues to https://github.com/cloudflare/workers-sdk/issues/new/choose`;


Nit, but a won't always be appropriate here (e.g. is a alpha command)

andyjessop · 2024-09-12T08:12:52Z

packages/wrangler/src/index.ts

- wrangler.command("kv", `🗂️ Manage Workers KV Namespaces`, (kvYargs) => {
- return registerKvSubcommands(kvYargs, subHelp);
- });
+ register.registerNamespace("kv");


Would this be better named commands?:

commands.registerNamespace("kv");

is more descriptive.

packages/wrangler/src/core/define-command.ts

packages/wrangler/src/core/register-commands.ts

andyjessop · 2024-09-12T09:08:47Z

packages/wrangler/src/core/define-command.ts

+export const COMMAND_DEFINITIONS: Array<
+ CommandDefinition | NamespaceDefinition | AliasDefinition
+> = [];


I was wondering this too. What if they're used in multiple places, like in separate isolated packages?

To avoid any potential issues with shared state, and to make it more portable, we could create an index file here and export functions with closed state:

export const { defineCommand, defineNamespace, defineAlias } = createCommandDefineFns(); // terrible name...

This could then be created in each package where we want to isolate this behaviour, or could be used in the main wrangler package just as we're doing here in kv.

implement subhelp

andyjessop

This is great work. I love the way that commands are searchable by their string value, and the args validation is extracted from the handler.

andyjessop · 2024-10-07T11:43:00Z

packages/wrangler/CONTRIBUTING.md

+});
+```
+
+2. Command specific (named positional) args vs Shared args vs Global args


Suggested change

2. Command specific (named positional) args vs Shared args vs Global args

2. Command-specific (named positional) args vs shared args vs global args

andyjessop reviewed Aug 20, 2024

View reviewed changes

packages/wrangler/src/versions/deploy.ts Outdated Show resolved Hide resolved

penalosa linked an issue Aug 21, 2024 that may be closed by this pull request

Establish source of truth for beta commands #4178

Open

RamIdeas force-pushed the register-command branch from 68ce564 to 1103b89 Compare September 4, 2024 20:09

andyjessop reviewed Sep 5, 2024

View reviewed changes

packages/wrangler/CONTRIBUTING.md Outdated Show resolved Hide resolved

edmundhung reviewed Sep 5, 2024

View reviewed changes

packages/wrangler/CONTRIBUTING.md Outdated Show resolved Hide resolved

packages/wrangler/src/core/teams.d.ts Outdated Show resolved Hide resolved

packages/wrangler/src/__tests__/core/command-registration.test.ts Outdated Show resolved Hide resolved

RamIdeas force-pushed the register-command branch 4 times, most recently from 8209c19 to c17ca68 Compare September 6, 2024 14:43

RamIdeas added the e2e Run e2e tests on a PR label Sep 6, 2024

RamIdeas marked this pull request as ready for review September 6, 2024 16:27

RamIdeas requested a review from a team as a code owner September 6, 2024 16:27

RamIdeas force-pushed the register-command branch from 958415a to 348152c Compare September 9, 2024 15:27

penalosa reviewed Sep 9, 2024

View reviewed changes

RamIdeas force-pushed the register-command branch from b730836 to 67876f5 Compare September 11, 2024 18:41

andyjessop reviewed Sep 12, 2024

View reviewed changes

RamIdeas commented Sep 12, 2024

View reviewed changes

packages/wrangler/src/core/define-command.ts Outdated Show resolved Hide resolved

andyjessop reviewed Sep 12, 2024

View reviewed changes

RamIdeas added 9 commits September 23, 2024 15:50

defineCommand interface

26888d0

implement registerAllCommands util

6296439

log deprecation/status message before running command handler

236b3f4

add tests

04a4fd8

chore

2568a6b

cleanup

f80504d

implement registerNamespace

c18ee5c

add contrib guide

80f2af1

add validateArgs

660f34f

RamIdeas added 19 commits September 23, 2024 15:50

fix: don't register commands more than once per run

b5cfced

implement subhelp

refactor KV command registration

a1fc7ff

update snapshots

a89e314

prettier

749b22f

implement behaviour.printBanner option

03375c2

disable printBanner for some kv commands to match existing behaviour

2a5802a

add custom deprecation messages to match existing behaviour

9fa6d8c

chore

53626c6

update docs

bbbc1ce

fix: arg types

11a52a5

chore: rename

babcd1c

chore: no need to return yargs

afdb683

chore: rename

ab157d4

update snapshot

4db5687

chore: DefineCommandResult

1d8ed43

remove behaviour.sendMetrics

de66985

Update define-command.ts

647246d

refactor

77839c2

lints

6112fe4

RamIdeas force-pushed the register-command branch from 0bf9160 to 6112fe4 Compare September 23, 2024 14:58

fix lint

ca3f113

andyjessop approved these changes Oct 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

defineCommand util #6526

defineCommand util #6526

RamIdeas commented Aug 19, 2024 •

edited

Loading

changeset-bot bot commented Aug 19, 2024 •

edited

Loading

github-actions bot commented Aug 21, 2024 •

edited

Loading

penalosa Sep 9, 2024

andyjessop Sep 12, 2024

andyjessop Sep 12, 2024

andyjessop left a comment •

edited

Loading

andyjessop Oct 7, 2024

	2. Command specific (named positional) args vs Shared args vs Global args
	2. Command-specific (named positional) args vs shared args vs global args

defineCommand util #6526

Are you sure you want to change the base?

defineCommand util #6526

Conversation

RamIdeas commented Aug 19, 2024 • edited Loading

What this PR solves / how to test

Author has addressed the following

changeset-bot bot commented Aug 19, 2024 • edited Loading

⚠️ No Changeset found

github-actions bot commented Aug 21, 2024 • edited Loading

penalosa Sep 9, 2024

Choose a reason for hiding this comment

andyjessop Sep 12, 2024

Choose a reason for hiding this comment

andyjessop Sep 12, 2024

Choose a reason for hiding this comment

andyjessop left a comment • edited Loading

Choose a reason for hiding this comment

andyjessop Oct 7, 2024

Choose a reason for hiding this comment

RamIdeas commented Aug 19, 2024 •

edited

Loading

changeset-bot bot commented Aug 19, 2024 •

edited

Loading

github-actions bot commented Aug 21, 2024 •

edited

Loading

andyjessop left a comment •

edited

Loading