[NEXT]: webpack-cli serve refactor draft by knagaitsev · Pull Request #1011 · webpack/webpack-cli

knagaitsev · 2019-07-28T00:36:19Z

What kind of change does this PR introduce?

Initial draft for #900

This makes webpack-cli serve use the new command-line-args argument parsing system, and it makes this command call the webpack-dev-server API, rather than the CLI.

Did you add tests for your changes?

No

If relevant, did you update the documentation?

No

Summary

The goal of this change is ultimately to make webpack-cli serve work directly with the webpack-dev-server API. I have placed this API call in startDevServer, with this file mostly copied from bin/webpack-dev-server.js. Ideally, we can minimalize what this helper does.

This draft is still very incomplete, what still needs to be done/considered:

Add all webpack-dev-server flags
Make the system use the correct webpack config file
Is it best that this works mostly independently from the new webpack-cli system?
make false work as a CLI flag definition, like with yargs where you could do --inline=false for bools

Reorganization:

the dev server flags I think should live in the webpack-dev-server repository, but it is easier to draft them here.

Edit:

We will remove the --color CLI flag in favor of simply retrieving the colors option from the compiler options.
The --info and --quiet flags will also be removed

Does this PR introduce a breaking change?

TBD

Other information

hiroppy · 2019-07-28T03:20:00Z

@@ -0,0 +1,93 @@
+const fs = require('fs');


Why do you use js?

@hiroppy Just easier for me to draft it faster. Can change it later once plan is more solidified.

alexander-akait

Let's discuss before continue work

alexander-akait · 2019-07-29T09:06:39Z

+ throw err;
+ });
+ }
+}


/cc @Loonride we need discussion about this, keep this stuff here will not allow us to change logic and fix very quickly in the future, we need create api on webpack-dev-server side, maybe idea with new api method is not bad

@evilebottnawi I agree, I simply copied and pasted it because I knew I would probably be changing it on the webpack-dev-server side. An alternative to a new API method is introducing some minor breaking changes to the listen method, as I started doing here: webpack/webpack-dev-server#2061, so that the listen method works without any additional configuration for the CLI. I think either approach could work.

@Loonride yes, let's finish this in webpack-dev-server and continue development here 👍

evenstensberg · 2019-07-30T20:24:35Z

- if (name === 'serve') {
- return pkgLoc ? require(pkgLoc).serve(args) : null;
- }
+// if (name === 'serve') {


Instead of uncommenting, just remove it after wip

knagaitsev · 2019-07-31T04:56:13Z

I've made it so that webpack-cli serve now uses the compiler that the webpack-cli command would create.

knagaitsev · 2019-08-01T00:55:33Z

A few thoughts:

should serve package use async/await?
at what point should the Promise resolve that is returned by the serve package? Currently, it resolves once the compiler is retrieved, but does not wait for anything else. Should it wait until both the server is serving, and the first compilation has completed?

knagaitsev · 2019-08-02T02:40:05Z

@@ -0,0 +1,23 @@
+import * as Server from "webpack-dev-server/lib/Server";


/cc @evilebottnawi I made some progress here. In here I put basically the ideal version of what we want, so minimal options handling before starting the server.
I think flags.js should live in webpack-dev-server in the future, rather than here, because then we can easily make updates without changing webpack-cli, but we can put that in webpack-dev-servernext later. Do you agree?
Edit: Also, should CLI option tests live in webpack-dev-server, then spawn the webpack-cli serve command?

The tests for options should live inside webpack-cli - since it owns the corresponding API for said options.
webpack-dev-server should have tests that validates integration & compatibility with webpack-cli api. For ex, this would apply to your changes for webpack-cli/lib/webpack-cli.js where you have added the processArgs fn, etc.

knagaitsev · 2019-08-08T21:51:42Z

/cc @evilebottnawi @hiroppy Thoughts? I know I still may need to make changes here based on dev server changes, but I think that, optimally, there should be very minimal interaction with dev server options as I have done here.

Where should tests be? Should tests related to running the basic webpack-cli serve command be in webpack-cli, and commands related to specific CLI args be in webpack-dev-server?

Where should the list of dev server CLI flags be? I think webpack-dev-server, so that they can easily be maintained.

alexander-akait · 2019-09-05T10:36:56Z

+ multiple: true,
+ },
+ ],
+};


/cc @Loonride we should avoid this file

It will be changed in next version

evenstensberg

Do you mind rebasing such that you only have the changes to the serve command and related?

knagaitsev · 2019-09-07T21:36:59Z

Do you mind rebasing such that you only have the changes to the serve command and related?

@evenstensberg I did rebase. But I don't think Files Changed is any different. Did you see a problem with something?

knagaitsev · 2019-09-22T15:32:13Z

I'm ready to get this merged to the next branch if you rebase @Loonride

@evenstensberg I rebased. I've moved the dev server flags into the lib directory temporarily only to pass CI (It is complaining about it being a .js file). Eventually, we will move this file out of webpack-cli and into webpack-dev-server, but it is convenient to keep it here for now when making CLI changes.

Edit: It seems typescript is still complaining about the .js files, but I didn't alter the tsconfig at all. Any idea how to fix this:

error TS5055: Cannot write file '/home/travis/build/webpack/webpack-cli/lib/commands/external.js' because it would overwrite input file.

jamesgeorge007 · 2019-09-23T16:30:47Z

 const isFlagPresent = (args, flag) => args.find(arg => [flag, `--${flag}`].includes(arg));
-const stripDashedFlags = (args, cmd) => args.slice(2).filter(arg => ~arg.indexOf('--') && arg !== cmd.name && arg !== cmd.alias);
+const isArgCommandName = (arg, cmd) => arg === cmd.name || arg === cmd.alias;
+const stripDashedFlags = (args, cmd) => args.filter(arg => ~arg.indexOf('--') && !isArgCommandName(arg, cmd));


Suggested change
conststripDashedFlags=(args,cmd)=>args.filter(arg=>~arg.indexOf('--')&&!isArgCommandName(arg,cmd));
conststripDashedFlags=(args,cmd)=>args.filter(arg=>arg.includes('--')&&!isArgCommandName(arg,cmd));

Actually I think the logic got messed up and I just followed along with it 🙁
It should be !arg.includes('--'), because we want to get rid of all the arguments with -- in this helper, correct?

evenstensberg · 2019-09-24T01:39:07Z

@Loonride that might be something with your tsconfig file, try tweaking around with it and running npm run build and npm run bootstrap

webpack-bot · 2019-09-25T19:23:02Z

@Loonride Please review the following output log for errors:

Worker information [...]
Build system information [...]
travis_fold:start:docker_mtu
travis_fold:end:docker_mtu
travis_fold:start:resolvconf
travis_fold:end:resolvconf
Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
Setting environment variables from repository settings
$ export nproc=4
Setting environment variables from .travis.yml
$ export JOB_PART=integration
Setting up build cache [...]
[...]
$ node --version
v8.16.1
$ npm --version
6.4.1
$ nvm --version
0.34.0
$ npm run travis:lint
> webpack-cli@4.0.0-alpha-5 travis:lint /home/travis/build/webpack/webpack-cli
> npm run build && npm run lint
> webpack-cli@4.0.0-alpha-5 build /home/travis/build/webpack/webpack-cli
> tsc
error TS5055: Cannot write file '/home/travis/build/webpack/webpack-cli/lib/commands/external.js' because it would overwrite input file.
error TS5055: Cannot write file '/home/travis/build/webpack/webpack-cli/lib/groups/help.js' because it would overwrite input file.
error TS5055: Cannot write file '/home/travis/build/webpack/webpack-cli/lib/utils/cli-flags.js' because it would overwrite input file.
error TS5055: Cannot write file '/home/travis/build/webpack/webpack-cli/lib/utils/compiler.js' because it would overwrite input file.
error TS5055: Cannot write file '/home/travis/build/webpack/webpack-cli/lib/utils/dev-config.js' because it would overwrite input file.
error TS5055: Cannot write file '/home/travis/build/webpack/webpack-cli/lib/utils/dev-server-flags.js' because it would overwrite input file.
error TS5055: Cannot write file '/home/travis/build/webpack/webpack-cli/lib/utils/group-helper.js' because it would overwrite input file.
error TS5055: Cannot write file '/home/travis/build/webpack/webpack-cli/lib/utils/interactive.js' because it would overwrite input file.
error TS5055: Cannot write file '/home/travis/build/webpack/webpack-cli/lib/utils/prod-config.js' because it would overwrite input file.
error TS5055: Cannot write file '/home/travis/build/webpack/webpack-cli/lib/utils/zero-config.js' because it would overwrite input file.
error TS5055: Cannot write file '/home/travis/build/webpack/webpack-cli/lib/webpack-cli.js' because it would overwrite input file.
Found 11 errors.
The command "npm run travis:lint" exited with 1.
$ npm run test:ci
> webpack-cli@4.0.0-alpha-5 test:ci /home/travis/build/webpack/webpack-cli
> nyc jest --maxWorkers=$(nproc) --reporters=default --reporters=jest-junit --colors
PASS test/utils/test-utils.test.js
PASS packages/webpack-scaffold/__tests__/index.test.ts (8.022s)
PASS packages/utils/__tests__/ast-utils.test.ts (9.735s)
PASS packages/utils/__tests__/resolve-packages.test.ts
PASS packages/utils/__tests__/package-manager.test.ts
PASS test/version/version-multi-args.test.js (6.688s)
PASS test/entry/defaults-index/entry-multi-args.test.js (7.389s)
PASS packages/info/__tests__/index.test.ts
PASS packages/utils/__tests__/validate-identifier.test.ts
PASS test/help/help-single-arg.test.js
PASS packages/migrate/__tests__/migrate.test.ts
PASS packages/utils/__tests__/recursive-parser.test.ts
FAIL test/config/basic/basic-config.test.js
● basic config file › is able to understand and parse a very basic configuration file
expect(received).toBe(expected) // Object.is equality
Expected: null
Received: [Error: ENOENT: no such file or directory, stat '/home/travis/build/webpack/webpack-cli/test/config/basic/binary/a.bundle.js']
15 | expect(outDirToMatch).toContain(outputDir);
16 | stat(resolve(__dirname, './binary/a.bundle.js'), (err, stats) => {
> 17 | expect(err).toBe(null);
| ^
18 | expect(stats.isFile()).toBe(true);
19 | done();
20 | });
at stat (test/config/basic/basic-config.test.js:17:25)
PASS test/config/empty/emptyConfig.test.js
PASS test/json/basic/basic.test.js
PASS test/entry/defaults-empty/entry-single-arg.test.js
PASS test/config/type/array/array-config.test.js
PASS test/config/type/function/function-config.test.js
PASS packages/migrate/commonsChunkPlugin/__tests__/commonsChunkPlugin.test.ts
PASS test/help/help-multi-args.test.js
PASS packages/utils/__tests__/npm-packages-exists.test.ts
PASS packages/utils/__tests__/is-local-path.test.ts
PASS packages/migrate/loaders/__tests__/loaders.test.ts
PASS test/version/version-single-arg.test.js
Please remove deprecated plugins manually. See https://webpack.js.org/guides/migrating/ for more information.
Please remove deprecated plugins manually. See https://webpack.js.org/guides/migrating/ for more information.
Please remove deprecated plugins manually. See https://webpack.js.org/guides/migrating/ for more information. PASS packages/migrate/removeDeprecatedPlugins/__tests__/removeDeprecatedPlugins.test.ts
PASS packages/generators/__tests__/add-generator.test.ts
PASS packages/migrate/uglifyJsPlugin/__tests__/uglifyJsPlugin.test.ts
PASS packages/generators/__tests__/loader-generator.test.ts
PASS packages/migrate/loaderOptionsPlugin/__tests__/loaderOptionsPlugin.test.ts
PASS packages/utils/__tests__/npm-exists.test.ts
PASS packages/migrate/removeJsonLoader/__tests__/removeJsonLoader.test.ts
PASS packages/migrate/moduleConcatenationPlugin/__tests__/moduleConcatenationPlugin.test.ts
PASS packages/migrate/noEmitOnErrorsPlugin/__tests__/noEmitOnErrorsPlugin.test.ts
PASS packages/migrate/namedModulesPlugin/__tests__/namedModulesPlugin.test.ts
PASS packages/migrate/bannerPlugin/__tests__/bannerPlugin.test.ts
PASS packages/migrate/outputPath/__tests__/outputPath.test.ts
PASS packages/migrate/extractTextPlugin/__tests__/extractTextPlugin.test.ts
PASS packages/migrate/resolve/__tests__/resolve.test.ts
�[999D�[K
Summary of all failing tests
FAIL test/config/basic/basic-config.test.js
● basic config file › is able to understand and parse a very basic configuration file
expect(received).toBe(expected) // Object.is equality
Expected: null
Received: [Error: ENOENT: no such file or directory, stat '/home/travis/build/webpack/webpack-cli/test/config/basic/binary/a.bundle.js']
15 | expect(outDirToMatch).toContain(outputDir);
16 | stat(resolve(__dirname, './binary/a.bundle.js'), (err, stats) => {
> 17 | expect(err).toBe(null);
| ^
18 | expect(stats.isFile()).toBe(true);
19 | done();
20 | });
at stat (test/config/basic/basic-config.test.js:17:25)
Test Suites: 1 failed, 1 skipped, 37 passed, 38 of 39 total
Tests: 1 failed, 1 skipped, 174 passed, 176 total
Snapshots: 96 passed, 96 total
Time: 47.876s
Ran all test suites.
The command "npm run test:ci" exited with 1.
store build cache [...]
Done. Your build exited with 1.

See complete report here.

evenstensberg

Small comments and then this is ready, thank you so much @Loonride

evenstensberg · 2019-09-26T21:07:53Z

+/**
+ *
+ * Converts CLI args to camel case from dash-separated words
+ *


could you make this filename hyphen based instead?

knagaitsev · 2019-09-26T21:12:53Z

Small comments and then this is ready, thank you so much @Loonride

I'm still not sure about the failing CI @evenstensberg

evenstensberg · 2019-09-26T21:15:32Z

Don't worry about it, is my fault, will deal with that

evenstensberg

lgtm, baller! Thank you so much for the amazing effort you've put into this and the dev-server 💙

alexander-akait · 2019-09-27T11:04:41Z

/cc @evenstensberg please revert, some problem not solved 😞

alexander-akait

😞

alexander-akait · 2019-09-27T11:12:43Z

+ multiple: true,
+ },
+ ],
+};


/cc @Loonride it is very very very bad, next PR will change a lot of options, we don't need this do in webpack-cli

alexander-akait · 2019-09-27T11:14:36Z

+ }
+ if (typeof onListening === 'function') {
+ onListening();
+ }


We should solve this on webpack-dev-server side, not here

We should solve this on webpack-dev-server side, not here
This onListening is different, it is just a callback as one of the function parameters. It can be removed if needed.

alexander-akait · 2019-09-27T11:15:43Z

+ */
+export default function startDevServer(compiler, options, onListening): void {
+ const firstWpOpt = compiler.compilers
+ ? compiler.compilers[0].options


Why only 0? What about multi-mode compilation

@evilebottnawi In the past on webpack-dev-server we only looked at the first compiler in a multi-compiler array to see if it has the devServer object. Do you want to change this behavior?

evenstensberg · 2019-09-27T17:11:27Z

@Loonride could you address these in a new PR?

webpack-bot added PR: unreviewed PR: CI-not-ok labels Jul 28, 2019

hiroppy reviewed Jul 28, 2019
View reviewed changes

webpack-bot added PR: conflict and removed PR: CI-not-ok labels Jul 29, 2019

alexander-akait requested changes Jul 29, 2019
View reviewed changes

webpack-bot added PR: reviewed-changes-requested PR: review-outdated and removed PR: unreviewed PR: reviewed-changes-requested labels Jul 29, 2019

knagaitsev force-pushed the feat/serve-wip branch from 1fbee3c to 3908bccCompare July 29, 2019 22:32

webpack-bot removed the PR: conflict label Jul 29, 2019

knagaitsev changed the title ~~NEXT: webpack-cli serve refactor draft (WIP)~~[NEXT]: webpack-cli serve refactor draft (WIP)Jul 29, 2019

webpack-bot added the PR: CI-not-ok label Jul 29, 2019

evenstensberg reviewed Jul 30, 2019
View reviewed changes

knagaitsev commented Aug 2, 2019
View reviewed changes

knagaitsev mentioned this pull request Aug 6, 2019
refactor(options): Move options from createConfig to normalizeOptions webpack/webpack-dev-server#2174
Closed
6 tasks

knagaitsev force-pushed the feat/serve-wip branch from d1e0eb1 to 4682770Compare August 19, 2019 23:35

knagaitsev requested a review from a team as a code owner August 19, 2019 23:35

alexander-akait requested changes Sep 5, 2019
View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: review-outdated labels Sep 5, 2019

evenstensberg reviewed Sep 7, 2019
View reviewed changes

knagaitsev force-pushed the feat/serve-wip branch from 2633835 to 1544a12Compare September 7, 2019 21:32

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels Sep 7, 2019

knagaitsev added 2 commits September 22, 2019 10:18

cli(bootstrap): change helper functions slightly and fix style
b681514

misc(serve): moved dev server flags temporarily to lib folder
c13f05b

knagaitsev force-pushed the feat/serve-wip branch from 8e2d3d9 to c13f05bCompare September 22, 2019 15:27

jamesgeorge007 suggested changes Sep 23, 2019
View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: review-outdated labels Sep 23, 2019

chore(cli): fix helper to use includes for dashed flag stripping
9da9db4

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

chore(serve): allow js in serve package
7e38b31

evenstensberg reviewed Sep 26, 2019
View reviewed changes

knagaitsev added 2 commits September 26, 2019 16:13

style(serve): make args to camel case helper hyphen based filename
6568984

style(bootstrap): return early and remove else from helper
4ba468b

chore(serve): remove allowjs from tsconfig
3c92b0a

evenstensberg approved these changes Sep 26, 2019
View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Sep 26, 2019

evenstensberg changed the title ~~[NEXT]: webpack-cli serve refactor draft (WIP)~~[NEXT]: webpack-cli serve refactor draftSep 26, 2019

evenstensberg merged commit 10790c8 into webpack:nextSep 26, 2019

alexander-akait reviewed Sep 27, 2019
View reviewed changes

knagaitsev mentioned this pull request Oct 8, 2019
chore(serve): remove promise return from serve package #1091
Merged

		@@ -0,0 +1,23 @@
		import * as Server from "webpack-dev-server/lib/Server";

	conststripDashedFlags=(args,cmd)=>args.filter(arg=>~arg.indexOf('--')&&!isArgCommandName(arg,cmd));
	conststripDashedFlags=(args,cmd)=>args.filter(arg=>arg.includes('--')&&!isArgCommandName(arg,cmd));

Uh oh!

Uh oh!

Conversation

knagaitsev commented Jul 28, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knagaitsev commented Jul 31, 2019

Uh oh!

knagaitsev commented Aug 1, 2019

Uh oh!

knagaitsevAug 2, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knagaitsev commented Aug 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evenstensberg left a comment

Choose a reason for hiding this comment

Uh oh!

knagaitsev commented Sep 7, 2019

Uh oh!

knagaitsev commented Sep 22, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evenstensberg commented Sep 24, 2019

Uh oh!

webpack-bot commented Sep 25, 2019

Uh oh!

evenstensberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

knagaitsev commented Sep 26, 2019

Uh oh!

evenstensberg commented Sep 26, 2019

Uh oh!

evenstensberg left a comment

Choose a reason for hiding this comment

Uh oh!

alexander-akait commented Sep 27, 2019

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

knagaitsev commented Jul 28, 2019•
edited
Loading

knagaitsevAug 2, 2019•
edited
Loading

knagaitsev commented Sep 22, 2019•
edited
Loading