Fixed Wrangler invalid command / argument error logging #12050
Conversation
🦋 Changeset detectedLatest commit: 5f7cdff 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 |
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: |
NuroDev
changed the title
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (hasHelpFlag && subCommand) { | ||
| const knownCommands = registry.topLevelCommands; | ||
| if (!knownCommands.has(subCommand)) { | ||
| logger.info(""); |
There was a problem hiding this comment.
This is here simply to appease formatting to match other errors by adding a spacing around the error.
| * Set of legacy command names registered outside the `CommandRegistry` class. | ||
| * Used to track commands like `containers`, `pubsub`, etc. | ||
| */ | ||
| #legacyCommands: Set<string>; |
There was a problem hiding this comment.
Why can't they all be moved to the registry ?
There was a problem hiding this comment.
Most commands are registered with the registry, but a handful of others still use the wrangler.command(...) system and we need to aggregate them both into a single source of truth.
To keep this PR simple(r) and not break anything I didn't want to move them to the registry yet. It could be simple, but I'm still not extremely well versed in the CLI setup to know if legacy commands require something that the registry doesn't?
There was a problem hiding this comment.
I had a similar question on a PR from TK yesterday.
My opinion is that we should stop adding things in the wrong way(TM) and refactor first - unless there is an urgent need to do otherwise.
Having different code paths is also something that also impacts the other PR.
+100 to refactor before doing more changes that will make the needed refactor harder later.
/cc @MattieTK @petebacondarwin
edit: I think the following somehow proves my point:
I'm still not extremely well versed in the CLI setup to know if legacy commands require something that the registry doesn't?
Yes the current code is too complex.
What we should do is to simplify/unify it instead of making if more complex for the next one that will debug/edit it.
This reverts commit 059bb56.
|
Spoke with @petebacondarwin yesterday about this and I'm going to put this PR on hold until #12055 is merged so this fix doesn't have to rely on legacy commands. |
2 participants
Fixes #11823
Before
After
Before
After
A picture of a cute animal (not mandatory, but encouraged)