Skip to content

fix(serve): do not default port in webpack-dev-server v4 #2126

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

Merged
merged 2 commits into from
Nov 24, 2020

Conversation

ylemkimon
Copy link
Contributor

@ylemkimon ylemkimon commented Nov 20, 2020

What kind of change does this PR introduce?
Bug fix and refactor

Did you add tests for your changes?
Unit tests. Integration tests will be possible once after webpack-dev-server v4 has been released.

If relevant, did you update the documentation?
N/A

Summary
With webpack/webpack-dev-server#2845, webpack-dev-server will handle setting the default port.

Depends on the PR above.

Does this PR introduce a breaking change?
N/A

Other information

@codecov
Copy link

codecov bot commented Nov 20, 2020

Codecov Report

Merging #2126 (1b3b16a) into master (cf5d1cf) will increase coverage by 0.04%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2126      +/-   ##
==========================================
+ Coverage   68.65%   68.70%   +0.04%     
==========================================
  Files          74       74              
  Lines        2393     2390       -3     
  Branches      510      512       +2     
==========================================
- Hits         1643     1642       -1     
+ Misses        750      748       -2     
Impacted Files Coverage Δ
packages/serve/src/startDevServer.ts 87.87% <90.90%> (+2.16%) ⬆️
packages/serve/src/createConfig.ts 55.55% <100.00%> (-9.16%) ⬇️

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 cf5d1cf...1b3b16a. Read the comment docs.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Ideally webpack-cli should not set default options for any options

@@ -33,16 +39,21 @@ export default function startDevServer(compiler, devServerArgs): object[] {
devServerOptions.forEach((devServerOpts): void => {
const options = mergeOptions(cliOptions, devServerOpts);
options.host = options.host || 'localhost';
Copy link
Member

Choose a reason for hiding this comment

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

@ylemkimon I think we have a problem here too, I think we used 0.0.0.0 by default here, we should use the same logic as used in Node.js http/https module

Copy link
Member

Choose a reason for hiding this comment

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

But let's solve this in other PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing the default hostname would be a breaking change for webpack-cli.

Copy link
Member

Choose a reason for hiding this comment

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

Let's do it only for v4

Copy link
Contributor Author

@ylemkimon ylemkimon Nov 24, 2020

Choose a reason for hiding this comment

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

I think it's better done on the dev-server's side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but I think it is bug and we should fix it, ideally we should be like

const http = require('http');

const requestListener = function (req, res) {
    res.writeHead(200);
    res.end('Hello, World!');
}

const server = http.createServer(requestListener);
server.listen(8080);

console.log(server.address())

:: for IPv6
0.0.0.0 for IPv4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants