fix(serve): merge CLI and devServer options correctly #1649

knagaitsev · 2020-06-24T18:31:15Z

What kind of change does this PR introduce?

fix/refactor

Did you add tests for your changes?

Not yet: This will not exactly work without a new webpack-dev-server release

If relevant, did you update the documentation?

no

Summary

@anshumanv Sorry for taking over on this from #1470, just wanted to show what I'm thinking. The merge strategy for CLI options and the user's devServer options in their config should be that CLI options take precedence over devServer options.

This merge strategy relies on: webpack/webpack-dev-server#2659, where default values are removed from serve CLI flags, then default values are set later if needed.

We need to keep the createConfig stuff here as minimal as possible, because we want all of the heavy configuration work to be done on the webpack-dev-server side, but some config creation is still necessary on this side.

TODO: even though default CLI flag values are removed internally, we still need to make a way to display the values that are defaulted to when somebody runs webpack-cli help serve.

Also: need to remove the any types from this PR still.

Does this PR introduce a breaking change?

Other information

Things to think about:

How should types like the dev server options type be handled? Currently I made a types.ts file in the serve package, but such types should probably be in a separate package
We should add some e2e tests ensuring the dev server works with special flag cases (like --no-static, --static dir1 --static dir2, etc.), but we need to wait for webpack-dev-server v4 release for these CLI flag settings to be available to webpack-cli

alexander-akait

Need fix type error

rishabh3112

Left a suggestion for how should we be selecting compiler in case of multiple compilers.
Also for return types of functions (Types I suggested are not very strict, custom interfaces could be used as well)

rishabh3112 · 2020-06-27T08:48:12Z

packages/serve/src/getDevServerOptions.ts

+    if (compiler.compilers) {
+        // devServer options could be found in any of the compilers,
+        // so simply find the first instance and use it, if there is one
+        const comp = compiler.compilers.find((comp) => comp.options.devServer);


We should let user decide which one to use. There are two possible solutions for this.

Add an inquirer question with the names of compilers and let user select.

Add a --name flag where user can supply the name of the compiler that should be used. (as suggested by @evilebottnawi)

Using both is also a solution (a better one imo).

I think the name is better here, but in theory developers can run two/three and more dev servers, we need think about it, maybe we should run all possible dev server (configuration where devServer exists), if developers don't want it we provide the name option

Yeah @evilebottnawi, I have also suggested to run all possible devServers in parallel before, here is a small thread on the discussion.

Friendly ping @Loonride :)

Will get back to this soon, there is still quite a few things to do on the dev server side first 😄

I agree with the name idea. Maybe it should also take a number index for the compiler being used, in case the user has not given their compilers names?

Are you suggesting that we run multiple devServers on different ports and pass each individual devServer options in? I think it would make sense to throw an error if they don't specify unique ports for each config.

I agree with the name idea. Maybe it should also take a number index for the compiler being used, in case the user has not given their compilers names?

I think we should enforce name only for name flag instead of indices here as they are bound to change if we change order of compilers in subsequent calls.

Are you suggesting that we run multiple devServers on different ports and pass each individual devServer options in? I think it would make sense to throw an error if they don't specify unique ports for each config.

For a particular devServer, we assign it a port if it is there else look for a free port. Throwing errors in this case can be avoided leading to better UX.

P.S.
A possible issue (on CLI side) in this case would handling logging in CLI such that we know which compiler's devServer is logging on stdout of terminal, ( compiler name would be helpful here ).

Ok, I switched name to not act as an index.

I'm just concerned with ports that if there is a lot of CLI output, it will be difficult for the user to even figure out which ports the server is running on when ports are assigned automatically

I'm just concerned with ports that if there is a lot of CLI output, it will be difficult for the user to even figure out which ports the server is running on when ports are assigned automatically

Yeah, this would be problem if silent/quiet is configured by a user (in other cases we can log this metadata to terminal).
Should we log this metadata (like port, name, etc) any way in that case as well 😅?

Regarding finding output of a particular compiler/port among a lot CLI output, we can configure logging using name of compiler as it's identifier and have output something like concurrently.

rishabh3112 · 2020-06-27T08:49:39Z

packages/serve/src/mergeOptions.ts

+ *
+ * @returns {Object} merged options object
+ */
+export default function mergeOptions(cliOptions, devServerOptions): any {


Suggested change

export default function mergeOptions(cliOptions, devServerOptions): any {

export default function mergeOptions(cliOptions, devServerOptions): object {

rishabh3112 · 2020-06-27T08:50:06Z

packages/serve/src/getDevServerOptions.ts

+ *
+ * @returns {Object}
+ */
+export default function getDevServerOptions(compiler): any {


Suggested change

export default function getDevServerOptions(compiler): any {

export default function getDevServerOptions(compiler): object {

rishabh3112 · 2020-06-27T08:50:48Z

packages/serve/src/createConfig.ts

+ *
+ * @returns {Object} valid devServer options object
+ */
+export default function createConfig(args): any {


Suggested change

export default function createConfig(args): any {

export default function createConfig(args): object {

alexander-akait · 2020-07-03T12:27:11Z

/cc @Loonride let's review this, it is blocker for future improvements

knagaitsev · 2020-07-07T17:52:59Z

packages/serve/src/types.ts

@@ -0,0 +1,5 @@
+export type devServerOptionsType = {


It would be ideal to have a full dev server config type once this is worked on: #866 and once we have finished major dev server options changes

We already have it here but needs to be updated, and I don't want to be importing the generators package just for this https://github.com/webpack/webpack-cli/blob/next/packages/generators/src/types/index.ts

@Loonride friendly ping

Sorry, need to focus on webpack-dev-server v4 first, will get back to this

gabriel-peracio · 2020-08-03T14:49:49Z

This is a bit of a blocker for my workflow, is there anything I can do to help move this PR along?

knagaitsev · 2020-08-17T05:27:13Z

My apologies, if anyone desperately needs this done feel free to take it over, otherwise I will look at it again in a few days

knagaitsev · 2020-08-21T20:37:43Z

@evilebottnawi There is an issue here with the name flag as a way for the user to select which devServer config they want. Say that a user has a config multi.config.js:

module.exports = [
  {
    name: 'foo',
    mode: 'development',
    entry: './foo.js',
    output: {
      filename: 'foo.js',
    },
    devServer: {...},
  },
  {
    name: 'bar',
    mode: 'development',
    entry: './bar.js',
    output: {
      filename: 'bar.js',
    },
    devServer: {...},
  },
];

Say the user runs: webpack-cli serve --name foo --config multi.config.js.

Based on what we said above, this should result in only the devServer from the foo config being used.

However, current behavior seems to be that both compilers get their names set to foo. Is this expected behavior? Since the --name flag already has a purpose, it seems we need a new flag name for the user to select which compiler devServer config they want to use.

Maybe --serve-name or something to make it identifiable that this flag is choosing which devServer config to use.

anshumanv · 2020-08-21T20:41:51Z

@Loonride
we already have --config-name to select config on the CLI

knagaitsev · 2020-08-21T22:41:19Z

@Loonride
we already have --config-name to select config on the CLI

This might be a bit of an edge case, but what if the user still wants the multi compiler config on the webpack side, but then they only want to select the devServer config from one of those compilers? Using --config-name it seems would only use 1 compiler from the list of compilers, so it wouldn't be running the multi compiler setup. That's what I was originally envisioning this option being for, but not sure if it is worth it.

Edit: anyway, I will finish the main goal of this PR and we can look at new features in other PRs

snitin315 · 2020-08-22T01:45:04Z

Using --config-name it seems would only use 1 compiler from the list of compilers, so it wouldn't be running the multi compiler setup

--config-name supports selecting multiple compilers => webpack --config-name foo --config-name bar

knagaitsev · 2020-08-22T16:49:42Z

Using --config-name it seems would only use 1 compiler from the list of compilers, so it wouldn't be running the multi compiler setup

--config-name supports selecting multiple compilers => webpack --config-name foo --config-name bar

I see, but the question still remains that if both foo and bar have a devServer config then how can the user select only 1 of these devServer configs to run, not both?

alexander-akait · 2020-08-24T09:51:56Z

@Loonride Ready for review?

knagaitsev · 2020-08-24T19:27:45Z

@Loonride Ready for review?

@evilebottnawi Not quite yet, still adding tests

knagaitsev · 2020-08-25T05:37:22Z

/cc @evilebottnawi Ready for review

Basically what I have done is:

broken everything into helper functions for easier testing, and added full testing
make multiple flags and a String or Boolean flag work correctly, as needed here: fix(cli): fix certain flags for webpack-cli serve webpack-dev-server#2722
update tests for arg-parser of webpack-cli

Edit: seems I still need to investigate CI problems. It seems the issue is related to webpack@next ProgressPlugin not working with webpack-dev-server v3.10.3

knagaitsev · 2020-08-26T06:49:19Z

/cc @evilebottnawi ready for review

webpack-bot · 2020-08-26T12:21:08Z

@anshumanv Thanks for your update.

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

@snitin315 Please review the new changes.

knagaitsev requested a review from a team as a code owner June 24, 2020 18:31

webpack-bot added the PR: unreviewed label Jun 24, 2020

anshumanv mentioned this pull request Jun 24, 2020

fix: pass devServer args to server options #1470

Closed

alexander-akait reviewed Jun 26, 2020

View reviewed changes

rishabh3112 reviewed Jun 27, 2020

View reviewed changes

lukereative mentioned this pull request Jun 30, 2020

Serve command not passing webpack devServer config to webpack-dev-server #1469

Closed

rishabh3112 mentioned this pull request Jul 7, 2020

Support setting the config name for multiple configs #1657

Closed

2 tasks

knagaitsev commented Jul 7, 2020

View reviewed changes

knagaitsev closed this Aug 17, 2020

knagaitsev reopened this Aug 17, 2020

webpack-bot added the PR: conflict label Aug 18, 2020

knagaitsev mentioned this pull request Aug 21, 2020

BREAKING CHANGE: remove CLI default flag values for webpack-cli serve webpack/webpack-dev-server#2659

Merged

6 tasks

knagaitsev force-pushed the fix/dev-server-options-merging branch from 29e507b to c6357a1 Compare August 21, 2020 17:48

webpack-bot removed the PR: conflict label Aug 21, 2020

knagaitsev force-pushed the fix/dev-server-options-merging branch from c6357a1 to a75e611 Compare August 21, 2020 17:50

knagaitsev mentioned this pull request Aug 24, 2020

fix(cli): fix certain flags for webpack-cli serve webpack/webpack-dev-server#2722

Merged

6 tasks

knagaitsev force-pushed the fix/dev-server-options-merging branch from bf2f415 to 5debcfd Compare August 25, 2020 05:23

knagaitsev changed the title ~~[WIP] fix(serve): merge CLI and devServer options correctly~~ fix(serve): merge CLI and devServer options correctly Aug 25, 2020

knagaitsev added 6 commits August 25, 2020 15:34

tests(serve): start making tests for startDevServer

036d6e1

chore(serve): add new parseArgs helper

ff84bcb

tests(serve): finish parsedArgs tests

95066f1

tests(serve): completed startDevServer tests

8a3bf07

tests(serve): update parseArgs snapshots for webpack-dev-server v3

17f77bd

tests(serve): remove webpack as dev dependency for serve

f9bbefc

knagaitsev force-pushed the fix/dev-server-options-merging branch from e5cdef7 to f9bbefc Compare August 25, 2020 20:34

knagaitsev added 5 commits August 25, 2020 20:14

chore(serve): add comment about how merge options should work

6824fb6

chore(serve): add webpack as dep

68a9802

chore(serve): remove webpack as serve dep

a183450

tests(serve): update parseArgs snapshot

c6ebda9

chore(serve): add comment about use of option in test

9661231

rishabh3112 approved these changes Aug 26, 2020

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: unreviewed labels Aug 26, 2020

alexander-akait approved these changes Aug 26, 2020

View reviewed changes

snitin315 approved these changes Aug 26, 2020

View reviewed changes

Merge branch 'next' into fix/dev-server-options-merging

6f52de9

webpack-bot added PR: review-outdated and removed PR: reviewed-approved labels Aug 26, 2020

anshumanv approved these changes Aug 26, 2020

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Aug 26, 2020

jamesgeorge007 approved these changes Aug 26, 2020

View reviewed changes

snitin315 approved these changes Aug 26, 2020

View reviewed changes

anshumanv merged commit 2cdf5ce into webpack:next Aug 26, 2020

Christian24 mentioned this pull request Feb 4, 2021

serve does not work with multi-server #2408

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(serve): merge CLI and devServer options correctly #1649

fix(serve): merge CLI and devServer options correctly #1649

knagaitsev commented Jun 24, 2020 •

edited

Loading

alexander-akait left a comment

rishabh3112 left a comment

rishabh3112 Jun 27, 2020

alexander-akait Jul 3, 2020 •

edited

Loading

rishabh3112 Jul 3, 2020

knagaitsev Jul 6, 2020

knagaitsev Jul 6, 2020 •

edited

Loading

rishabh3112 Jul 7, 2020

knagaitsev Jul 7, 2020

rishabh3112 Jul 8, 2020

rishabh3112 Jun 27, 2020

rishabh3112 Jun 27, 2020

rishabh3112 Jun 27, 2020

alexander-akait commented Jul 3, 2020

knagaitsev Jul 7, 2020

rishabh3112 Jul 14, 2020

knagaitsev Jul 22, 2020

gabriel-peracio commented Aug 3, 2020

knagaitsev commented Aug 17, 2020

knagaitsev commented Aug 21, 2020

anshumanv commented Aug 21, 2020

knagaitsev commented Aug 21, 2020 •

edited

Loading

snitin315 commented Aug 22, 2020

knagaitsev commented Aug 22, 2020

alexander-akait commented Aug 24, 2020

knagaitsev commented Aug 24, 2020

knagaitsev commented Aug 25, 2020 •

edited

Loading

knagaitsev commented Aug 26, 2020

webpack-bot commented Aug 26, 2020

	export default function mergeOptions(cliOptions, devServerOptions): any {
	export default function mergeOptions(cliOptions, devServerOptions): object {

	export default function getDevServerOptions(compiler): any {
	export default function getDevServerOptions(compiler): object {

	export default function createConfig(args): any {
	export default function createConfig(args): object {

fix(serve): merge CLI and devServer options correctly #1649

fix(serve): merge CLI and devServer options correctly #1649

Conversation

knagaitsev commented Jun 24, 2020 • edited Loading

alexander-akait left a comment

Choose a reason for hiding this comment

rishabh3112 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait Jul 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knagaitsev Jul 6, 2020 • edited Loading

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 commented Jul 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabriel-peracio commented Aug 3, 2020

knagaitsev commented Aug 17, 2020

knagaitsev commented Aug 21, 2020

anshumanv commented Aug 21, 2020

knagaitsev commented Aug 21, 2020 • edited Loading

snitin315 commented Aug 22, 2020

knagaitsev commented Aug 22, 2020

alexander-akait commented Aug 24, 2020

knagaitsev commented Aug 24, 2020

knagaitsev commented Aug 25, 2020 • edited Loading

knagaitsev commented Aug 26, 2020

webpack-bot commented Aug 26, 2020

knagaitsev commented Jun 24, 2020 •

edited

Loading

alexander-akait Jul 3, 2020 •

edited

Loading

knagaitsev Jul 6, 2020 •

edited

Loading

knagaitsev commented Aug 21, 2020 •

edited

Loading

knagaitsev commented Aug 25, 2020 •

edited

Loading