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: Check both IPv4 and IPv6 when dns-name supplied #84

Merged
merged 2 commits into from
Sep 8, 2022
Merged

fix: Check both IPv4 and IPv6 when dns-name supplied #84

merged 2 commits into from
Sep 8, 2022

Conversation

danez
Copy link
Contributor

@danez danez commented Aug 15, 2022

This change resolves issues with Node.js 17 and newer and fixes #81.

Node.js 17 and newer do not order ip addresses when resolving them, this means that a server that is listening on localhost previously always listened on 127.0.0.1 with the Node.js 17+ it might also be listening on ::1. To support this new behavior, wait-port now tries to connect to IPv4 first and then to IPv6 when the supplied hostname is not an IP.

I change the return value to include the ipVersion and because of this this is a breaking change.

BREAKING CHANGE: Returns now an object instead of an boolean. The object
contains the property open: boolean and if open is true it will
also contain ipVersion which will be 4 or 6 dependening on which IP
version the open port was found on.

@danez
Copy link
Contributor Author

danez commented Aug 26, 2022

Hi @dwmkerr, sorry for the ping, but could you take a look at this?

@dwmkerr
Copy link
Owner

dwmkerr commented Sep 5, 2022

Apologies for the delay @danez and thanks for this change! I'm checking now

@dwmkerr
Copy link
Owner

dwmkerr commented Sep 5, 2022

Hi @danez would you mind updating from main? I've fixed up the build pipeline so that it builds with Node 12, 14, 16, 18, I think the PR you've made might fail 12/14 but not 16 onwards, this should confirm. It might be that the code that handles IP v4/6 needs to be conditional dependent on the node platform

BREAKING CHANGE: Returns now an object instead of an boolean. The object
contains the property `open: boolean` and if `open` is `true` it will
also contain `ipVersion` which will be `4` or `6` dependening on which IP
version the open port was found on.
@danez
Copy link
Contributor Author

danez commented Sep 5, 2022

I rebased

@dwmkerr
Copy link
Owner

dwmkerr commented Sep 6, 2022

Thanks @danez running the builds now

@dwmkerr
Copy link
Owner

dwmkerr commented Sep 6, 2022

Looks like the tests are failing @danez I checked locally on each node version in the build and got the same failures, are you seeing the same?

@danez
Copy link
Contributor Author

danez commented Sep 8, 2022

Sorry for the confusion, I checked the tests and noticed I forgot to update them. Because this PR changes the return value to an object instead of a boolean.

@dwmkerr
Copy link
Owner

dwmkerr commented Sep 8, 2022

No worries @danez ! Testing now and will fingers crossed then release shortly!

@codecov
Copy link

codecov bot commented Sep 8, 2022

Codecov Report

Merging #84 (71bb14d) into main (fd4dc5f) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #84      +/-   ##
==========================================
+ Coverage   92.98%   93.06%   +0.08%     
==========================================
  Files           9        9              
  Lines         171      173       +2     
==========================================
+ Hits          159      161       +2     
  Misses         12       12              
Impacted Files Coverage Δ
lib/wait-port.js 89.81% <100.00%> (+0.19%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@dwmkerr dwmkerr merged commit 3c3821c into dwmkerr:main Sep 8, 2022
@dwmkerr
Copy link
Owner

dwmkerr commented Sep 8, 2022

Amazing, thanks for this @danez !

@danez danez deleted the ipv6 branch October 12, 2022 08:26
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.

Newest version of library doesn't work with CentOS7
2 participants