Skip to content
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.

Sign up for GitHub

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: pass devServer args to server options #1470

Closed
wants to merge 3 commits into from
Closed

fix: pass devServer args to server options #1470

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bca25a9
fix: pass devServer args to server config
anshumanv Apr 16, 2020
a42a892
fix: devServer config takes precedence
anshumanv Apr 16, 2020
6d2cfc8
Merge branch 'next' into server/config
anshumanv Jun 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions packages/serve/src/startDevServer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 || {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can run for both of them in parallel as they both would be configured for different entry points. wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the merge strategy?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// set default host and port
devServerOptions = { host: 'localhost', port: 8080, ...devServerOptions };

const host = options.host || devServerOptions.host || 'localhost';
const port = options.port || devServerOptions.port || 8080;
// socket should not have a default value, because it should only be used if the
// user explicitly provides it
const socket = options.socket || devServerOptions.socket;

options.host = host;
options.port = port;
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 };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @Loonride how do we proceed here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you get any findings about this @Loonride ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, I can look at this more closely now that we are close to webpack-dev-server v4

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, let me know your thoughts :)


const { host, port } = serverConfig;

const server = new Server(compiler, serverConfig);
server.listen(socket || port, host, (err): void => {
if (err) {
throw err;
Expand Down