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.

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: use the host in options to check if port is available #4385

Merged
merged 1 commit into from
Apr 17, 2022

Conversation

ludofischer
Copy link
Contributor

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Yes, but it is hard to test for non-localhost hosts.

Motivation / Use-Case

The server should check if the port is available on the host specified by the user.

Breaking Changes

No breaking changes.

Additional Info

Copy link
Contributor Author

@ludofischer ludofischer left a comment

Choose a reason for hiding this comment

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

These changes should result in the same code executing as before if the host is localhost. I think the case where someone wants the dev server to listen on non-localhost must be fairly rare.

@@ -716,9 +717,9 @@ declare class Server {
description: string;
multiple: boolean;
path: string;
type: string;
type: string /** @type {WebSocketURL} */;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regenerating the Server types introduces a lot unrelated comments in the type definitions, but running build:types on master does not create any diff.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, types changed here since you added * @param {string} host

Copy link
Member

Choose a reason for hiding this comment

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

It is bug in ts, reported about it, but still not fixed

@codecov
Copy link

codecov bot commented Apr 14, 2022

Codecov Report

Merging #4385 (c95b18a) into master (eea50f3) will increase coverage by 0.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4385      +/-   ##
==========================================
+ Coverage   92.27%   92.42%   +0.14%     
==========================================
  Files          16       16              
  Lines        1593     1597       +4     
  Branches      596      598       +2     
==========================================
+ Hits         1470     1476       +6     
+ Misses        114      112       -2     
  Partials        9        9              
Impacted Files Coverage Δ
lib/Server.js 93.66% <100.00%> (ø)
lib/getPort.js 95.83% <100.00%> (+0.37%) ⬆️
lib/servers/WebsocketServer.js 94.87% <0.00%> (+5.12%) ⬆️

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 eea50f3...c95b18a. Read the comment docs.

Copy link
Member

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@@ -406,7 +407,7 @@ class Server {
? parseInt(process.env.WEBPACK_DEV_SERVER_PORT_RETRY, 10)
: 3;

return pRetry(() => getPort(basePort), {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure retry is going to help a lot as the getPort() (and portfinder) before is going to try every port up to the maximum port number anyway, but it was introduced on purpose in this commit back in 2019, so there might be a reason.

@alexander-akait alexander-akait merged commit a10c7cf into webpack:master Apr 17, 2022
@alexander-akait
Copy link
Member

Thanks

@ludofischer ludofischer deleted the pass-host branch April 17, 2022 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants