-
-
Notifications
You must be signed in to change notification settings - Fork 653
chore: use commander built-ins over custom implementations #4642
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.
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
Conversation
f92627e to
ad96398
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the webpack CLI to rely on Commander’s built-in help, error handling, and unknown-command behavior instead of custom implementations, while simplifying option wiring across built-in and external commands.
Changes:
- Rewrote
WebpackCLI.runand related plumbing to construct commands/options via Commander primitives, usingshowHelpAfterError,exitOverride, andunknownCommandrather than custom help/validation logic. - Simplified
makeCommand/makeOptionAPIs and types, and updated built-in commands (build,watch,serve,info,configtest) plus their tests and snapshots to match Commander’s output and suggestion formats. - Removed the custom help rendering path and the
fastest-levenshteindependency, tightened spell-check configuration, and updated tests/snapshots for help and unknown-command/option behavior.
Reviewed changes
Copilot reviewed 13 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
packages/webpack-cli/src/webpack-cli.ts |
Core refactor: configures Commander output/exit behavior, rebuilds built-in commands and options using makeCommand/makeOption, and hooks global --help / --version / color flags into Commander instead of custom parsing. |
packages/webpack-cli/src/types.ts |
Updates CLI types to match the new makeCommand/makeOption contract, introduces WebpackCallback typing for compiler callbacks, and adjusts logger and option metadata types. |
packages/webpack-cli/src/bootstrap.ts |
Simplifies CLI bootstrap to call cli.run(args) and adds a small helper to auto-run when invoked from an npm_lifecycle_script named tsx. |
packages/webpack-cli/package.json |
Removes the fastest-levenshtein dependency now that Commander drives unknown-option suggestions. |
packages/serve/src/index.ts |
Changes ServeCommand.apply to accept pre-built options, derive dev-server flags via cli.webpack.cli.getArguments, and register serve using the new makeCommand / makeOption flow. |
packages/info/src/index.ts |
Registers info via makeCommand, wiring its options through cli.makeOption, and extends aliases to include version/v so these commands reuse the info implementation. |
packages/configtest/src/index.ts |
Registers configtest via the new makeCommand API, leaving the validation logic intact but delegating CLI wiring to the shared helpers. |
test/help/help.test.js |
Prunes now-redundant help tests and adjusts the remaining tests to assert equivalence between --help and help command syntax plus the new invalid --help=<value> behavior. |
test/help/__snapshots__/help.test.js.snap.devServer5.webpack5 |
Updates help snapshots to match Commander’s default help layout and wording for the root and command-specific help output. |
test/build/unknown/__snapshots__/unknown.test.js.snap.webpack5 |
Refreshes snapshots for unknown flags/commands to reflect Commander’s suggestion format and colorized stderr output. |
test/build/basic/basic.test.js |
Updates expectations for unknown-command messaging (e.g., Unknown command 'buil' and new suggestion phrasing) consistent with Commander’s output. |
test/api/CLI.test.js |
Reworks CLI API tests to use the new makeCommand/makeOption signatures, exercising a wide variety of option shapes/types against the refactored wiring. |
test/api/__snapshots__/CLI.test.js.snap.webpack5 |
Removes the snapshot for the now-deleted custom help output, since help is delegated to Commander. |
package-lock.json |
Syncs lockfile with dependency changes (notably removing fastest-levenshtein and marking a dependency as peer: true). |
.cspell.json |
Tightens spell-check configuration, enabling useGitignore and simplifying ignore paths to avoid checking snapshots and package manifests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| warn: (val, raw) => log("warn", this.colors.red(util.format(val)), raw), | ||
| info: (val, raw) => log("info", this.colors.red(util.format(val)), raw), | ||
| success: (val, raw) => log("log", this.colors.red(util.format(val)), raw), | ||
| log: (val, raw) => log("error", this.colors.red(util.format(val)), raw), |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new getLogger implementation appears to regress the log levels and output streams: warn, info, and success all use this.colors.red(...), and log delegates to log("error", ...), which routes messages to stderr. Previously these methods used different colors and log went to standard output, and other parts of the code (e.g. CLIPlugin.setupHelpfulOutput) expect logger.log to behave as a general info logger, not an error logger. Please restore distinct colors for warn/info/success and have log write to stdout (e.g. via console.log or the log branch of the helper), to avoid misclassified and mis-colored output.
| warn: (val, raw) => log("warn", this.colors.red(util.format(val)), raw), | |
| info: (val, raw) => log("info", this.colors.red(util.format(val)), raw), | |
| success: (val, raw) => log("log", this.colors.red(util.format(val)), raw), | |
| log: (val, raw) => log("error", this.colors.red(util.format(val)), raw), | |
| warn: (val, raw) => log("warn", this.colors.yellow(util.format(val)), raw), | |
| info: (val, raw) => log("info", this.colors.cyan(util.format(val)), raw), | |
| success: (val, raw) => log("log", this.colors.green(util.format(val)), raw), | |
| log: (val, raw) => log("log", this.colors.cyan(util.format(val)), raw), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why it was changed if you write - Commander's help and unknownCommand handlers? How logger related to these changes?
| declare interface WebpackCallback { | ||
| (err: null | Error, result?: Stats): void; | ||
| (err: null | Error, result?: MultiStats): void; | ||
| } | ||
|
|
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WebpackCallback is declared twice in this file (once at lines 29-32 and again here at 79-82) with identical signatures. The duplicate declaration is redundant and can be confusing for maintainers; please remove one of them and keep a single definition that is exported as needed.
| declare interface WebpackCallback { | |
| (err: null | Error, result?: Stats): void; | |
| (err: null | Error, result?: MultiStats): void; | |
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4642 +/- ##
=======================================
Coverage 95.65% 95.65%
=======================================
Files 2 2
Lines 23 23
Branches 4 4
=======================================
Hits 22 22
Misses 1 1 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of unrelated changed, at lease split PRs into small steps (commits), we have stable CLI over a lot of year and I sorry I can't review this code because a lot of different changes mixed in the one PR
|
At least please make logical commits and we can read what and where and why was changed |
| makeOption(command: WebpackCLICommand, option: WebpackCLIBuiltInOption) { | ||
| let mainOption: WebpackCLIMainOption; | ||
| let negativeOption; | ||
| makeOption(option: WebpackCLIBuiltInOption): WebpackCLICommandOption[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why makeOption was changed? Again - it is only Commander's help and unknownCommand handlers
| ...builtInFlags, | ||
| ...Object.entries(this.webpack.cli.getArguments()).map<WebpackCLIBuiltInOption>( | ||
| let coreOptions: WebpackCLIBuiltInOption[] = []; | ||
| if (this.webpack) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We always have webpack here
| if (builtInExternalCommandInfo) { | ||
| ({ pkg } = builtInExternalCommandInfo); | ||
| } else { | ||
| pkg = commandName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again - please split help changes and sumcommand refactor into two PRs (and maybe even more)
|
A lot of these changes are interconnected, but perhaps if it aint broken, there's no need to fix it. |
Summary
This is still quite a big PR, but I do believe it's an improvement. This PR changes the
webpack-cli.tsfile to use Commander'shelpandunknownCommandhandlers over the previous custom-implementation.What kind of change does this PR introduce?
This PR introduces a CLI change in the Commander program.
Did you add tests for your changes?
Nothing substantial changed that required additional tests, but all previous tests pass.
Note that tests that tested features that are no handled internally were removed, as those are tested by Commander, and don't need to be tested by us.
Also note that the commander unknown command output is slightly different from the previous implementation, so a few snapshots have been changed, but seeing as it's just one stderr line, I don't think it's a breaking change.
Does this PR introduce a breaking change?
As mentioned above, this PR does not include any breaking changes to CLI. The only difference is a slightly different output when an invalid command is passed (Commander gives recommendations in
()), but that's not breaking.