refactor(options): Move options from createConfig to normalizeOptions #2174

knagaitsev · 2019-08-03T04:58:38Z

For Bugs and Features; did you add new tests?

Not yet

Motivation / Use-Case

Currently conceptualizing how options handling should be moved into the API. There will be many minor breaking changes, but I think they will all ultimately be for the better.

Breaking Changes

Read the comments in normalizeOptions.

Additional Info

jsf-clabot · 2019-08-05T20:48:44Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

codecov · 2019-08-05T20:48:46Z

Codecov Report

Merging #2174 into next will decrease coverage by 0.34%.
The diff coverage is 89.28%.

@@            Coverage Diff             @@
##             next    #2174      +/-   ##
==========================================
- Coverage   93.57%   93.23%   -0.35%     
==========================================
  Files          33       33              
  Lines        1245     1256      +11     
  Branches      363      372       +9     
==========================================
+ Hits         1165     1171       +6     
- Misses         71       79       +8     
+ Partials        9        6       -3

Impacted Files	Coverage Δ
lib/utils/createConfig.js	`96.25% <ø> (-0.22%)`	⬇️
client-src/clients/BaseClient.js	`69.23% <ø> (ø)`	⬆️
client-src/clients/WebsocketClient.js	`55.26% <ø> (ø)`	⬆️
client-src/default/utils/getCurrentScriptSource.js	`100% <ø> (ø)`	⬆️
client-src/default/utils/createSocketUrl.js	`100% <ø> (ø)`	⬆️
client-src/clients/SockJSClient.js	`60.52% <ø> (ø)`	⬆️
client-src/default/utils/reloadApp.js	`100% <ø> (ø)`	⬆️
client-src/default/overlay.js	`96.82% <100%> (ø)`	⬆️
client-src/default/utils/log.js	`100% <100%> (ø)`	⬆️
lib/utils/runOpen.js	`100% <100%> (ø)`	⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0893f1...0965fbc. Read the comment docs.

codecov · 2019-08-05T20:48:46Z

Codecov Report

Merging #2174 into next will increase coverage by 0.14%.
The diff coverage is 96.12%.

@@            Coverage Diff             @@
##             next    #2174      +/-   ##
==========================================
+ Coverage   93.88%   94.02%   +0.14%     
==========================================
  Files          34       36       +2     
  Lines        1276     1323      +47     
  Branches      363      379      +16     
==========================================
+ Hits         1198     1244      +46     
+ Misses         71       70       -1     
- Partials        7        9       +2

Impacted Files	Coverage Δ
client-src/clients/BaseClient.js	`69.23% <ø> (-5.77%)`	⬇️
client-src/clients/SockJSClient.js	`60.00% <ø> (-1.54%)`	⬇️
client-src/clients/WebsocketClient.js	`58.97% <ø> (+1.07%)`	⬆️
client-src/default/utils/createSocketUrl.js	`100.00% <ø> (ø)`
client-src/default/utils/getCurrentScriptSource.js	`100.00% <ø> (ø)`
client-src/default/utils/reloadApp.js	`100.00% <ø> (ø)`
client-src/default/index.js	`91.56% <80.00%> (-0.66%)`	⬇️
lib/utils/createConfig.js	`98.70% <88.88%> (+2.24%)`	⬆️
lib/Server.js	`97.19% <95.45%> (-0.13%)`	⬇️
lib/utils/startUnixSocket.js	`96.55% <96.55%> (ø)`
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7daff1d...69aa911. Read the comment docs.

knagaitsev · 2019-08-06T03:45:31Z

/cc @evilebottnawi @hiroppy This is ready for review. I've moved many things from createConfig to normalizeOptions. The end goal, in my opinion, is to be able to remove createConfig completely, so that we can just take the parsed CLI args object and pass it straight into the API (as I am doing in webpack/webpack-cli#1011).

hiroppy · 2019-08-06T23:11:40Z

lib/utils/createConfig.js

-
-  if (options.open && !options.openPage) {
-    options.openPage = '';
+  // eslint-disable-next-line no-undefined


It seems unnecessary. Or if we need it, We'll be able move this rule to .eslintrc.

It is necessary, but I moved to the top of the createConfig file for convenience in this file. Let's remove the no-undefined requirement in another PR.

hiroppy · 2019-08-06T23:13:05Z

lib/utils/normalizeOptions.js

+  });
+
+  if (!options.publicPath) {
+    // eslint-disable-next-line


This was there originally, but I don't know why. eslint is fine without it.

hiroppy · 2019-08-06T23:20:22Z

lib/utils/normalizeOptions.js

+    options.inline = true;
+  }
+
+  // ---- The options below were already here, they are not breaking changes ---- //


This comment is needed?

My mistake, these comments were just helping me stay organized while working on this

hiroppy · 2019-08-06T23:20:31Z

test/server/utils/createConfig.test.js

@@ -210,131 +209,11 @@ describe('createConfig', () => {
    expect(config).toMatchSnapshot();
  });

-  it('publicPath option (not specify)', () => {
-    const config = createConfig(webpackConfig, argv, { port: 8080 });
+  // removed: publicPath tests


hiroppy · 2019-08-06T23:20:42Z

test/server/utils/createConfig.test.js

-
-    expect(config).toMatchSnapshot();
-  });
+  // removed: watchOptions tests


hiroppy · 2019-08-06T23:20:48Z

test/server/utils/createConfig.test.js

-
-    expect(config).toMatchSnapshot();
-  });
+  // removed: contentBase tests


knagaitsev · 2019-08-20T05:51:56Z

/cc @evilebottnawi @hiroppy Can you review this? I've moved most options from createConfig to normalizeOptions, skipping a few that will need some additional changes to be moved.

I've realized that one difficulty with the approach of merging the compiler's options.devServer with CLI options is default values for CLI. This is because you can't tell the difference between a CLI-set default value and a value that the user set explicitly. I'm hoping that maybe I can completely remove default CLI values on the CLI-side (with the exceptions probably just being host and port, as we need those for the listen method) then I can set default values in normalizeOptions if they are not explicitly set by the user.

hiroppy · 2019-08-20T18:15:02Z

I wanna wait for #2202 because this pr is related is-absolute-url.

hiroppy · 2019-08-20T18:17:21Z

I'm hoping that maybe I can completely remove default CLI values on the CLI-side

Agree. I hope so.

alexander-akait · 2019-08-22T10:45:54Z

lib/utils/createConfig.js

-const isAbsoluteUrl = require('is-absolute-url');
+/* eslint-disable
+  no-undefined
+*/


Let's avoid this in favor eslint-disable-next-line

alexander-akait · 2019-08-22T10:46:36Z

lib/utils/createConfig.js

  if (argv.bonjour) {
    options.bonjour = true;
  }

-  if (argv.host && (argv.host !== 'localhost' || !options.host)) {
+  if (argv.host) {


Why we remove && (argv.host !== 'localhost' || !options.host)?

We do not have to make this change now, but we need to carefully rethink how we handle the options: host, port, and socket, since these are relevant for the listen method.

The logic in && (argv.host !== 'localhost' || !options.host) is made difficult by the fact that localhost is the CLI default, so we can't tell if the user explicitly asked for localhost or if it was set by default:

webpack-dev-server/bin/options.js

Line 175 in eab6acd

default: 'localhost',

My proposal is:

Don't set default values for the CLI options host or port. I have done this in webpack-cli serve:
https://github.com/webpack/webpack-cli/blob/44d0fb01f47852b4dfc7423b8e1dfa6c51872e55/packages/serve/flags.js

Have a clear hierarchy for which value gets used. The order of precedence should be:

CLI value

webpackConfig.devServer value

default value

You can see I have implemented this order of precedence in these lines:
https://github.com/webpack/webpack-cli/pull/1011/files#diff-24921f08fe12bbbb2c25f5ed41d757b4R19-R23

Oh, i think we should keep https://github.com/webpack/webpack-cli/blob/44d0fb01f47852b4dfc7423b8e1dfa6c51872e55/packages/serve/flags.js on our side, because when we want add cli option it is require change something on webpack-cli side, it is not good

Oh, i think we should keep https://github.com/webpack/webpack-cli/blob/44d0fb01f47852b4dfc7423b8e1dfa6c51872e55/packages/serve/flags.js on our side, because when we want add cli option it is require change something on webpack-cli side, it is not good

I agree, but I don't think we should move it yet because it will be easier to finish implementing webpack-cli serve with the flags at webpack-cli, then we move it just before release.

i.e. webpack-cli will work with webpack-dev-server@3?

I was thinking that webpack-cli serve could just install the latest webpack-dev-server as a dependency (https://github.com/webpack/webpack-cli/pull/1011/files#diff-e630bd2ce7c76bdd3674ca4624f13b7fR18). Is that not what we want? I think it could be difficult to make refactored webpack-cli serve work with webpack-dev-server@3

@Loonride webpack-cli should not have webpack-dev-server in deps, they should use peerDependenciesMeta with optional: true

alexander-akait

Some notes

Because opn was renamed to open.

…ack#2178)

* feat(server): add stdin for api * test(stdin): switch to async await tests for stdin * test(cli): use await timer

knagaitsev added need tests pr: next labels Aug 3, 2019

knagaitsev requested review from alexander-akait and hiroppy as code owners August 3, 2019 04:58

hiroppy force-pushed the next branch from 148dfd5 to d0893f1 Compare August 5, 2019 17:44

knagaitsev force-pushed the migrate-createConfig-normalizeOptions branch from 0965fbc to eaf36fe Compare August 5, 2019 20:54

knagaitsev removed the need tests label Aug 6, 2019

This was referenced Aug 6, 2019

refactor(server): move filename option from createConfig to API #2113

Closed

refactor(server): move publicPath changes from CLI to API #2099

Closed

knagaitsev changed the title ~~refactor(options): Moving options from createConfig to normalizeOptions [WIP]~~ refactor(options): Move options from createConfig to normalizeOptions Aug 6, 2019

hiroppy reviewed Aug 6, 2019

View reviewed changes

knagaitsev added gsoc Google Summer of Code scope: cli labels Aug 13, 2019

alexander-akait reviewed Aug 22, 2019

View reviewed changes

hiroppy and others added 7 commits September 10, 2019 07:52

chore: don't support Node6 (webpack#1976)

06f6c53

refactor(client): make use of ESM instead of CJS (webpack#1967)

af76891

chore(deps): update dependency ws to v7 (webpack#1834)

fad833a

chore(deps): update dependency file-loader to v4 (webpack#1971)

b32f7a3

chore(deps): update dependency chokidar to v3 (webpack#1902)

342fa6a

chore(deps): introduce open and remove opn (webpack#1865)

7901428

Because opn was renamed to open.

chore: fix tests (webpack#1997)

898461a

hiroppy and others added 5 commits September 10, 2019 08:23

test: update syntax

1679797

refactor(server): remove some variables of options (webpack#2175)

c600b0e

chore(deps): update del, supports-color, import-local and execa (webp…

27b1913

…ack#2178)

feat(server): add stdin option to API (webpack#2186)

e819cfe

* feat(server): add stdin for api * test(stdin): switch to async await tests for stdin * test(cli): use await timer

refactor(server): move socket handling into server.listen (webpack#2061)

64036a0

hiroppy force-pushed the next branch from babdf28 to 64036a0 Compare September 10, 2019 03:13

knagaitsev added 12 commits September 10, 2019 19:01

refactor(options): wip moving options between helpers

97bf133

fix(cli): move back some options that must be in createConfig

1135766

fix(options): only map to absolute paths if not urls

9c087e5

test(options): fixed existing normalize opts tests

46a5b6c

test(server): add public path option tests to normalize tests

a54897b

test(options): add normalize options tests, remove create config tests

5d6502e

refactor(options): removed comments, changed eslint slightly

2f5f3b3

refactor(options): move public path option setting to single line

80f1f33

refactor(options): fix eslint double empty line, public path two lines

d942373

refactor(cli): move over more options from create config

0e5ede9

style(cli): add eslint no undefined for single line only

e7ab9f1

refactor(stats): moved stats and colors handling to normalize options

c674e87

knagaitsev force-pushed the migrate-createConfig-normalizeOptions branch from 105fdf9 to c674e87 Compare September 11, 2019 01:06

knagaitsev added 2 commits September 10, 2019 20:35

test(options): update normalize options snapshot

3e8d32c

test(options): mock supports color and fix transport mode

69aa911

hiroppy force-pushed the next branch 3 times, most recently from c54ae99 to edfcfd6 Compare December 19, 2019 09:07

hiroppy force-pushed the next branch 2 times, most recently from 4408469 to dc1c72f Compare December 24, 2019 05:53

alexander-akait force-pushed the next branch 2 times, most recently from d14892a to 5f1ad3b Compare January 31, 2020 15:58

alexander-akait closed this Feb 27, 2021

alexander-akait deleted the branch webpack:next February 27, 2021 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(options): Move options from createConfig to normalizeOptions #2174

refactor(options): Move options from createConfig to normalizeOptions #2174

knagaitsev commented Aug 3, 2019

jsf-clabot commented Aug 5, 2019 •

edited

Loading

codecov bot commented Aug 5, 2019

codecov bot commented Aug 5, 2019 •

edited

Loading

knagaitsev commented Aug 6, 2019

hiroppy Aug 6, 2019

knagaitsev Aug 7, 2019

hiroppy Aug 6, 2019

knagaitsev Aug 7, 2019

hiroppy Aug 6, 2019

knagaitsev Aug 7, 2019

hiroppy Aug 6, 2019

hiroppy Aug 6, 2019

hiroppy Aug 6, 2019

knagaitsev commented Aug 20, 2019

hiroppy commented Aug 20, 2019

hiroppy commented Aug 20, 2019

alexander-akait Aug 22, 2019

alexander-akait Aug 22, 2019

knagaitsev Aug 22, 2019

alexander-akait Aug 23, 2019

knagaitsev Aug 23, 2019

alexander-akait Aug 23, 2019

knagaitsev Aug 23, 2019

alexander-akait Aug 23, 2019 •

edited

Loading

alexander-akait left a comment

refactor(options): Move options from createConfig to normalizeOptions #2174

refactor(options): Move options from createConfig to normalizeOptions #2174

Conversation

knagaitsev commented Aug 3, 2019

For Bugs and Features; did you add new tests?

Motivation / Use-Case

Breaking Changes

Additional Info

jsf-clabot commented Aug 5, 2019 • edited Loading

codecov bot commented Aug 5, 2019

Codecov Report

codecov bot commented Aug 5, 2019 • edited Loading

Codecov Report

knagaitsev commented Aug 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knagaitsev commented Aug 20, 2019

hiroppy commented Aug 20, 2019

hiroppy commented Aug 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait Aug 23, 2019 • edited Loading

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

jsf-clabot commented Aug 5, 2019 •

edited

Loading

codecov bot commented Aug 5, 2019 •

edited

Loading

alexander-akait Aug 23, 2019 •

edited

Loading