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

chore: replace portfinder with custom implementation #4384

Merged
merged 2 commits into from
Apr 14, 2022

Conversation

ludofischer
Copy link
Contributor

@ludofischer ludofischer commented Apr 13, 2022

Fix #4383

  • Get rid of security warnings related to async dependencies
  • Drop lodash from prod.
  • The implementation is based mostly on the get-port package, but searches for ports incrementing by 1 like portfinder and I;ve added TypeScript annotations.
  • 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?

I've added new unit tests for the custom port finder implementation.

Motivation / Use-Case

Remove dependency on vulnerable package async, also remove dependency on lodash.

Breaking Changes

There should be no breaking changes.

Additional Info

I am in doubt whether to go with this or the get-port package as in https://github.com/ludofischer/webpack-dev-server/tree/switch-port-finder
The code is a bit complex because it needs to take into account differences across operating systems, so it might be useful to rely on the expertise of the get-port maintainters. On the other hand there are more test failures with the get-port package and we would be stuck on the previous get-port major version, since it switched to ESM-only.

Get rid of security warnings related to async dependencies
Drop lodash from prod.
The implementation is based mostly on the get-port package,
but searches for ports incrementing by 1 like portfinder.
@alexander-akait
Copy link
Member

The code is a bit complex because it needs to take into account differences across operating systems, so it might be useful to rely on the expertise of the get-port maintainters.

Can you clarify?

@codecov
Copy link

codecov bot commented Apr 13, 2022

Codecov Report

Merging #4384 (a421822) into master (c9b6433) will increase coverage by 0.09%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##           master    #4384      +/-   ##
==========================================
+ Coverage   92.18%   92.27%   +0.09%     
==========================================
  Files          15       16       +1     
  Lines        1549     1593      +44     
  Branches      591      596       +5     
==========================================
+ Hits         1428     1470      +42     
- Misses        112      114       +2     
  Partials        9        9              
Impacted Files Coverage Δ
lib/getPort.js 95.45% <95.45%> (ø)
lib/Server.js 93.66% <100.00%> (ø)

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 c9b6433...a421822. Read the comment docs.

@ludofischer
Copy link
Contributor Author

ludofischer commented Apr 13, 2022

Can you clarify?

For example you need to check the port on every local interface, else the port might look free even though it's already taken:

const getLocalHosts = () => {

Or the error codes you need to check for. This sort of thing someone who hasn't been maintaining a package like this for some time years will not understand.

@ludofischer
Copy link
Contributor Author

I also think there is a mistake in the current webpack-dev-server implementation. When you pass a custom host, the custom host is not passed to portfinder, so portfinder might check the ports on the wrong host.
See this line

this.options.host = await Server.getHostname(
and the implementation of Server.getFreePort().

@ludofischer ludofischer marked this pull request as ready for review April 13, 2022 20:46
@alexander-akait
Copy link
Member

alexander-akait commented Apr 14, 2022

I also think there is a mistake in the current webpack-dev-server implementation. When you pass a custom host, the custom host is not passed to portfinder, so portfinder might check the ports on the wrong host.

Yes, looks like a bug, let fix it too, do you want to fix it? If yes - here or in separate PR?

@ludofischer
Copy link
Contributor Author

Yes, looks like a bug, let fix it too, do you want to fix it? If yes - here or in separate PR?

SInce it's a longstanding bug I would fix it in a different PR. I think it's better to release the current PR on its own.

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

Thanks

@alexander-akait
Copy link
Member

Let's fix a problem with host and I will do patch release

@ludofischer ludofischer deleted the custom-port-finder branch April 14, 2022 17:38
@ludofischer
Copy link
Contributor Author

Let's fix a problem with host and I will do patch release

Do you mean fix the problem mentioned in #4384 (comment)?

@alexander-akait
Copy link
Member

@ludofischer Yep 👍

@ludofischer
Copy link
Contributor Author

@alexander-akait See #4385

@cjwilsontech
Copy link

Let's fix a problem with host and I will do patch release

@alexander-akait, any update on the patch release?

@alexander-akait
Copy link
Member

Yep, today 👍

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.

node-portfinder not supported anymore
3 participants