migrate to commander #1481

knagaitsev · 2020-04-18T18:57:15Z

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:

An argument that expects a string will throw an error if one is not provided (e.g. webpack-cli --output will throw an error because output expects a string value following it)
In the case of our --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 if opts.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

knagaitsev · 2020-04-18T20:51:57Z

/cc @evilebottnawi

alexander-akait · 2020-04-20T10:44:09Z

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.

Maybe we should throw an error? I think we should not support multiple same arguments for mode (and for some other arguments)

alexander-akait · 2020-04-20T10:45:43Z

packages/webpack-cli/package.json

@@ -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",


I think we don't need that package too

@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:

@Loonride Do we have API from commander for customize help output?

Do we have API from commander for customize help output?

Yes indeed, https://github.com/tj/commander.js#custom-help

alexander-akait · 2020-04-20T10:47:25Z

packages/serve/src/index.ts

-    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');


Maybe we can implement API method on webpack-cli side, like getFlagsForCommand('serve') and it is remove unnecessary argv?

Will be great to avoid working with argv on command side and process on command side. What do you think?

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

alexander-akait

Good job, I think we should improve some places

rishabh3112 · 2020-04-20T11:36:20Z

Maybe we should throw an error? I think we should not support multiple same arguments for mode (and for some other arguments)

Instead of error we should display warning. We should complete user's interaction where ever we can.

alexander-akait · 2020-04-20T11:43:08Z

@rishabh3112 hm, other CLI programs always throw an error on invalid arguments, but I am ok with warning

rishabh3112 · 2020-04-20T12:13:16Z

@evilebottnawi My point of view on this (sharing to validate my thinking as I might be wrong):
In this case we don't have invalid arguments, instead we we have dilemma which of the two same flags takes precedence. This can be resolved with a logical decision.
But yes, Many CLIs (including legacy ones) throw error in this case. But I think throwing error in this case is bad for DX, when we can logically imply what user wants to do and warn user with decision we took and what assumptions we took to make this decision.

knagaitsev · 2020-04-20T16:27:34Z

My personal opinion (feel free to disagree) is that we shouldn't have the --no-mode argument. It was introduced recently as described here: #1240

The same thing can already be done with --mode none
--no-argument generally implies to me that the argument is a boolean and you are switching it off
It adds ambiguities like if the user is doing --mode production --no-mode which need to be dealt with
commander doesn't support parse by telling us that --no-mode was provided so we would have to do our own parsing to see if the user has provided --mode and --no-mode (it simply sets --mode to false). Search for --no-cheese in here to see the behavior: https://github.com/tj/commander.js/

Edit: anyway for now I think I should just add the warning, and we can consider doing this for a later PR

knagaitsev · 2020-04-21T04:07:54Z

Notes about commander:

it seems if you want to negate a boolean flag with --no- it has to actually be added as an option along with the original boolean flag
there is no negating of a boolean via --flag=false, though you can set string flags with --flag=value.

TODO:

figure out how to handle negated flags for booleans
fix resolveNegatedArgs in bootstrap
add warning if user includes both --mode and --no-mode
~~add getFlagsForCommand('serve') and related tests~~ (no longer needed)
add tests to show --flag=value works
add tests for negated boolean flags and warnings related to them
add tests for arg-parser

What will be done in future PRs:

enabling --boolean-flag=false
potentially removing command-line-usage in favor of built-in commander help
add handling for allowed-hosts for serve (https://github.com/webpack/webpack-dev-server/blob/master/bin/cli-flags.js) using commander custom option parsing
remove --no-mode

alexander-akait · 2020-04-21T11:22:35Z

@Loonride

#1481 (comment)

Agree, let's remove --no-mode support 👍 We can do it after migration.

#1481 (comment)

Good job!

knagaitsev · 2020-04-24T17:43:17Z

/cc @Loonride Couple questions

@evilebottnawi all done

rishabh3112

Left a suggestion for a typo.

rishabh3112 · 2020-04-24T18:05:17Z

packages/webpack-cli/lib/utils/arg-parser.js

+        });
+    }
+
+    // Use customised help output is avaliable


Suggested change

// Use customised help output is avaliable

// Use customised help output if avaliable

snitin315 · 2020-04-24T18:17:11Z

packages/webpack-cli/lib/utils/arg-parser.js

+        });
+    }
+
+    // Use customised help output is avaliable


Suggested change

// Use customised help output is avaliable

// Use customised help output if available

jamesgeorge007 · 2020-04-25T05:35:40Z

test/stats/stats.test.js

@@ -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);


Unnecessary log.

alexander-akait

/cc @webpack/cli-team let's merge it 🎉

webpack-bot · 2020-04-25T15:09:21Z

@jamesgeorge007 Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@evilebottnawi Please review the new changes.

alexander-akait · 2020-04-25T15:47:46Z

/cc @jamesgeorge007

knagaitsev requested a review from a team as a code owner April 18, 2020 18:57

webpack-bot added the PR: unreviewed label Apr 18, 2020

knagaitsev force-pushed the chore/migrate-commander branch from 9f0c764 to ca94729 Compare April 18, 2020 18:59

alexander-akait reviewed Apr 20, 2020

View reviewed changes

rishabh3112 and others added 16 commits April 23, 2020 01:48

chore: use commander

e663d10

chore: add support for help and version command

87403bf

chore: remove duplicate code for no-mode

ec8c474

chore: create helpers

f7009ae

chore: remove global flag

af62829

chore: improve arg parsing and tests

eab3500

chore: remove command-line-args

ab17bf0

chore: remove instance of command-line-args

327fc7d

chore: add explaination to the change

05173d8

chore: fix

5244654

chore: migrate serve package

777f8e5

chore: handle error

b9a4932

chore: reactor argparser

ad0cc8f

chore: get serve args from process.argv

1b3212d

chore: sync message with test

503a1f7

chore: fix styling

7cf46bf

knagaitsev added 2 commits April 24, 2020 12:15

chore: add comment about commander negation

426c666

chore: made output tests sync

2deb7e3

rishabh3112 reviewed Apr 24, 2020

View reviewed changes

snitin315 reviewed Apr 24, 2020

View reviewed changes

chore: fix spelling in comment

dfb6f6c

jamesgeorge007 requested changes Apr 25, 2020

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: unreviewed labels Apr 25, 2020

chore: remove unnecessary log

c068011

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels Apr 25, 2020

chore: add runPromptWithAnswers changes for stability

9351796

alexander-akait approved these changes Apr 25, 2020

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Apr 25, 2020

Merge branch 'next' into chore/migrate-commander

d160106

webpack-bot added PR: review-outdated and removed PR: reviewed-approved labels Apr 25, 2020

rishabh3112 approved these changes Apr 25, 2020

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Apr 25, 2020

snitin315 approved these changes Apr 25, 2020

View reviewed changes

jamesgeorge007 approved these changes Apr 25, 2020

View reviewed changes

jamesgeorge007 merged commit 8892926 into webpack:next Apr 25, 2020

snitin315 mentioned this pull request Apr 27, 2020

refactor(webpack-cli): remove --no-mode flag #1503

Merged

anshumanv mentioned this pull request Apr 28, 2020

[WIP] chore: migrate to commander #1347

Closed

6 tasks

rishabh3112 added this to Done in webpack-cli v4 May 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate to commander #1481

migrate to commander #1481

knagaitsev commented Apr 18, 2020 •

edited

Loading

knagaitsev commented Apr 18, 2020

alexander-akait commented Apr 20, 2020

alexander-akait Apr 20, 2020

knagaitsev Apr 21, 2020 •

edited

Loading

alexander-akait Apr 21, 2020

jamesgeorge007 Apr 21, 2020 •

edited

Loading

alexander-akait Apr 20, 2020

alexander-akait Apr 20, 2020

knagaitsev Apr 24, 2020

alexander-akait left a comment

rishabh3112 commented Apr 20, 2020

alexander-akait commented Apr 20, 2020

rishabh3112 commented Apr 20, 2020

knagaitsev commented Apr 20, 2020 •

edited

Loading

knagaitsev commented Apr 21, 2020 •

edited

Loading

alexander-akait commented Apr 21, 2020

knagaitsev commented Apr 24, 2020

rishabh3112 left a comment

rishabh3112 Apr 24, 2020

snitin315 Apr 24, 2020 •

edited

Loading

jamesgeorge007 Apr 25, 2020

alexander-akait left a comment

webpack-bot commented Apr 25, 2020

alexander-akait commented Apr 25, 2020

	// Use customised help output is avaliable
	// Use customised help output if avaliable

	// Use customised help output is avaliable
	// Use customised help output if available

migrate to commander #1481

migrate to commander #1481

Conversation

knagaitsev commented Apr 18, 2020 • edited Loading

knagaitsev commented Apr 18, 2020

alexander-akait commented Apr 20, 2020

alexander-akait Apr 20, 2020

Choose a reason for hiding this comment

knagaitsev Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

Current webpack-cli help output using command-line-usage:

commander help:

alexander-akait Apr 21, 2020

Choose a reason for hiding this comment

jamesgeorge007 Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

alexander-akait Apr 20, 2020

Choose a reason for hiding this comment

alexander-akait Apr 20, 2020

Choose a reason for hiding this comment

knagaitsev Apr 24, 2020

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

rishabh3112 commented Apr 20, 2020

alexander-akait commented Apr 20, 2020

rishabh3112 commented Apr 20, 2020

knagaitsev commented Apr 20, 2020 • edited Loading

knagaitsev commented Apr 21, 2020 • edited Loading

alexander-akait commented Apr 21, 2020

knagaitsev commented Apr 24, 2020

rishabh3112 left a comment

Choose a reason for hiding this comment

rishabh3112 Apr 24, 2020

Choose a reason for hiding this comment

snitin315 Apr 24, 2020 • edited Loading

Choose a reason for hiding this comment

jamesgeorge007 Apr 25, 2020

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

webpack-bot commented Apr 25, 2020

alexander-akait commented Apr 25, 2020

knagaitsev commented Apr 18, 2020 •

edited

Loading

knagaitsev Apr 21, 2020 •

edited

Loading

Current webpack-cli help output using `command-line-usage`:

`commander` help:

jamesgeorge007 Apr 21, 2020 •

edited

Loading

knagaitsev commented Apr 20, 2020 •

edited

Loading

knagaitsev commented Apr 21, 2020 •

edited

Loading

snitin315 Apr 24, 2020 •

edited

Loading