-
-
Notifications
You must be signed in to change notification settings - Fork 597
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(serve): merge CLI and devServer options correctly #1649
fix(serve): merge CLI and devServer options correctly #1649
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need fix type error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah @evilebottnawi, I have also suggested to run all possible devServer
s in parallel before, here is a small thread on the discussion.
Friendly ping @Loonride :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will get back to this soon, there is still quite a few things to do on the dev server side first 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
packages/serve/src/mergeOptions.ts
Outdated
* | ||
* @returns {Object} merged options object | ||
*/ | ||
export default function mergeOptions(cliOptions, devServerOptions): any { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export default function mergeOptions(cliOptions, devServerOptions): any { | |
export default function mergeOptions(cliOptions, devServerOptions): object { |
* | ||
* @returns {Object} | ||
*/ | ||
export default function getDevServerOptions(compiler): any { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export default function getDevServerOptions(compiler): any { | |
export default function getDevServerOptions(compiler): object { |
packages/serve/src/createConfig.ts
Outdated
* | ||
* @returns {Object} valid devServer options object | ||
*/ | ||
export default function createConfig(args): any { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export default function createConfig(args): any { | |
export default function createConfig(args): object { |
/cc @Loonride let's review this, it is blocker for future improvements |
@@ -0,0 +1,5 @@ | |||
export type devServerOptionsType = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Loonride friendly ping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, need to focus on webpack-dev-server v4 first, will get back to this
This is a bit of a blocker for my workflow, is there anything I can do to help move this PR along? |
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 |
29e507b
to
c6357a1
Compare
c6357a1
to
a75e611
Compare
@evilebottnawi There is an issue here with the
Say the user runs: Based on what we said above, this should result in only the However, current behavior seems to be that both compilers get their names set to Maybe |
@Loonride |
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 Edit: anyway, I will finish the main goal of this PR and we can look at new features in other PRs |
|
I see, but the question still remains that if both |
@Loonride Ready for review? |
@evilebottnawi Not quite yet, still adding tests |
bf2f415
to
5debcfd
Compare
/cc @evilebottnawi Ready for review Basically what I have done is:
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 |
e5cdef7
to
f9bbefc
Compare
/cc @evilebottnawi ready for review |
@anshumanv Thanks for your update. I labeled the Pull Request so reviewers will review it again. @snitin315 Please review the new changes. |
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 thewebpack-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:
types.ts
file in the serve package, but such types should probably be in a separate package--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