fix: pass devServer args to server options #1470
Conversation
packages/serve/src/startDevServer.ts
Outdated
| if (socket) { | ||
| options.socket = socket; | ||
| } | ||
|
|
||
| const server = new Server(compiler, options); | ||
| // Compose config from the CLI and devServer object from webpack config | ||
| const serverConfig = { ...devServerOptions, ...options }; |
There was a problem hiding this comment.
Problem - this way we won't be able to override default options unless specified via flag. eg. clientLogLevel by default is set to 'info', so if we pass any other option in devServer, it won't take precedence, but we also need to support flags like --hot and override them with the devServer config so that's a dilemma
There was a problem hiding this comment.
CLI args should be override config values, what is a problem, can you clarify with examples?
There was a problem hiding this comment.
If we don't need to override config values then I think we're fine with devServer options taking precedence.
| if (socket) { | ||
| options.socket = socket; | ||
| } | ||
|
|
||
| const server = new Server(compiler, options); | ||
| // Compose config from the CLI and devServer object from webpack config | ||
| const serverConfig = { ...options, ...devServerOptions }; |
There was a problem hiding this comment.
I think I understand what you are saying. Do you mean the issue is that if a CLI flag has a default value, it will override devServer regardless of what the user has set for that value in devServer? Yes, this is an issue.
Our goal is that CLI args should take precedence over devServer, so this line here is not the perfect solution.
What we need to do is find a way to check if the user has actually provided the CLI flag, or if it is just getting passed in as a default value. It might be easier to hold off on this until we swap over to commander as the CLI flags parser. Ideally we can pass a CLI options object into startDevServer which indicates whether each flag was defaulted to, or whether the user explicitly provided the flag.
Alternatively, we should just not have dev server CLI flag default values.
There was a problem hiding this comment.
I think I understand what you are saying. Do you mean the issue is that if a CLI flag has a default value, it will override devServer regardless of what the user has set for that value in devServer? Yes, this is an issue.
Precisely.
That makes sense to me, lets revisit this after we're on the new parser.
There was a problem hiding this comment.
Can you find a case when we don't want the devServer config to override? I could think of --hot flag, when passed will appear in the CLI options object, and if devServer contains hot: false then the CLI option won't override this way, but nobody does will explicitly pass hot: false in their config right, since it's default?
There was a problem hiding this comment.
/cc @Loonride how do we proceed here?
There was a problem hiding this comment.
I think the goal may be to just make dev server flags not set default values. If we do that, this shouldn't be an issue. But in that case it would still be nice for users to see what values are defaulted to later on when they do like webpack-cli help serve. Will think about this a bit and come back to it
There was a problem hiding this comment.
Did you get any findings about this @Loonride ?
There was a problem hiding this comment.
Sorry for the delay, I can look at this more closely now that we are close to webpack-dev-server v4
There was a problem hiding this comment.
Awesome, let me know your thoughts :)
| @@ -11,21 +11,25 @@ import Server from 'webpack-dev-server/lib/Server'; | |||
| */ | |||
| export default function startDevServer(compiler, options): void { | |||
| const firstWpOpt = compiler.compilers ? compiler.compilers[0].options : compiler.options; | |||
| const devServerOptions = firstWpOpt.devServer || {}; | |||
| let devServerOptions = firstWpOpt.devServer || {}; | |||
There was a problem hiding this comment.
This is not entirely true. Second compiler also can contains devServer. To be honestly both can contain devServer, we need to think how to handle it, do we have --name option to choose compiler?
There was a problem hiding this comment.
I think we can run for both of them in parallel as they both would be configured for different entry points. wdyt?
There was a problem hiding this comment.
What would be the merge strategy?
There was a problem hiding this comment.
Assuming you are referring to merger of CLI options with config then I think you may use same strategy for each complier. Just CLI provided options would be same for each of them.
There was a problem hiding this comment.
I think we should look at all compilers, if you have two devServer output warning about you need specify name of compiler, in future we will dev server as plugin and we don't need this hack
|
Closing in favour of #1649 |
What kind of change does this PR introduce?
fix
Did you add tests for your changes?
Not yet.
If relevant, did you update the documentation?
NA
Summary
Does this PR introduce a breaking change?
No
Other information
Fixes #1469