-
-
Notifications
You must be signed in to change notification settings - Fork 597
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
migrate to commander #1481
migrate to commander #1481
Conversation
9f0c764
to
ca94729
Compare
/cc @evilebottnawi |
Maybe we should throw an error? I think we should not support multiple same arguments for mode (and for some other arguments) |
@@ -27,8 +27,8 @@ | |||
"@webpack-cli/info": "^1.0.1-alpha.4", | |||
"ansi-escapes": "^4.3.1", | |||
"chalk": "^3.0.0", | |||
"command-line-args": "^5.1.1", | |||
"command-line-usage": "^6.1.0", |
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.
I think we don't need that package too
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.
@evilebottnawi Good you pointed that out, we actually still rely on it for webpack-cli help
. It still functions correctly, since we didn't change the cli-flags
object format at all. I think it is fine actually to continue using it as it formats nicely and we can keep our current CLI flags object format is it is so that it continues to work with command-line-usage
.
The alternative is to start using the built in commander help, we would just need to do some work making the output look nicer. Here is the difference:
Current webpack-cli help output using command-line-usage
:
commander
help:
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.
@Loonride Do we have API from commander for customize help output?
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.
Do we have API from commander for customize help output?
Yes indeed, https://github.com/tj/commander.js#custom-help
packages/serve/src/index.ts
Outdated
const devServerArgs = cli.commandLineArgs(devServer, { argv: args, partial: true }); | ||
const webpackArgs = cli.commandLineArgs(core, { argv: devServerArgs._unknown || [], stopAtFirstUnknown: false }); | ||
const finalArgs = argsToCamelCase(devServerArgs._all || {}); | ||
const filteredArgs = process.argv.filter((arg) => arg != 'serve'); |
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.
Maybe we can implement API method on webpack-cli side, like getFlagsForCommand('serve') and it is remove unnecessary argv?
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.
Will be great to avoid working with argv on command side and process on command side. What do you think?
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.
actually this is not needed, it seems the filtered args are already passed into the serve command and I just wasn't aware of it
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.
Good job, I think we should improve some places
Instead of error we should display warning. We should complete user's interaction where ever we can. |
@rishabh3112 hm, other CLI programs always throw an error on invalid arguments, but I am ok with warning |
@evilebottnawi My point of view on this (sharing to validate my thinking as I might be wrong): |
My personal opinion (feel free to disagree) is that we shouldn't have the
Edit: anyway for now I think I should just add the warning, and we can consider doing this for a later PR |
Notes about commander:
TODO:
What will be done in future PRs:
|
Agree, let's remove Good job! |
@evilebottnawi all done |
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.
Left a suggestion for a typo.
}); | ||
} | ||
|
||
// Use customised help output is avaliable |
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.
// Use customised help output is avaliable | |
// Use customised help output if avaliable |
}); | ||
} | ||
|
||
// Use customised help output is avaliable |
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.
// Use customised help output is avaliable | |
// Use customised help output if available |
test/stats/stats.test.js
Outdated
@@ -48,7 +48,8 @@ describe('stats flag', () => { | |||
it('should warn when an unknown flag stats value is passed', () => { | |||
const { stderr, stdout } = run(__dirname, ['--stats', 'foo']); | |||
expect(stderr).toBeTruthy(); | |||
expect(stderr).toContain('No value recognised for "stats" option'); | |||
console.log(stderr); |
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.
Unnecessary log.
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.
/cc @webpack/cli-team let's merge it 🎉
@jamesgeorge007 Thanks for your update. I labeled the Pull Request so reviewers will review it again. @evilebottnawi Please review the new changes. |
/cc @jamesgeorge007 |
What kind of change does this PR introduce?
refactor
Did you add tests for your changes?
Yes, updating tests
If relevant, did you update the documentation?
No
Summary
Picking up from #1347
Looking to get this done so we can move forward with
webpack-cli serve
.Does this PR introduce a breaking change?
Breaking changes to adapt to the norms of
commander
:webpack-cli --output
will throw an error becauseoutput
expects a string value following it)--mode
and--no-mode
arguments, the--no-mode
argument actually functions in commander to set the--mode
argument to false. Therefore, we can check if--no-mode
has been set by checking ifopts.mode === false
. The quirk here is that if both are used, the one that comes last takes precedence. So if a user does--mode production --no-mode
, then mode will be false, but if the user does--no-mode --mode production
, mode will be production.Other information