-
-
Notifications
You must be signed in to change notification settings - Fork 40
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
Force HTTP check under corresponding protocols #56
Conversation
CI is failing |
The failed test tests |
index.js
Outdated
@@ -38,10 +38,11 @@ const checkHttp = async (url, timeout) => { | |||
const getAddress = async hostname => net.isIP(hostname) ? hostname : (await dnsLookupP(hostname)).address; | |||
|
|||
const isTargetReachable = timeout => async target => { | |||
const isHTTP = ['http:', 'https:'].some(prot => target.indexOf(prot) === 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is overly complicated.
const isHTTP = ['http:', 'https:'].some(prot => target.indexOf(prot) === 0); | |
const isHTTP = target.startsWith('https://') || target.startsWith('http://'); |
Isn't that what we are already doing now? |
I would recommend "git blaming" it and check why it was added. |
Since URL object with http or https protocol and default port(80 or 443) will not show the port, the port is only assigned to default port 443 when the protocol is neither one of them.
#38 removed support for other well-known protocol, so it changed the test of ftp url to the current one. |
Fixes #55.
Since URL object with http or https protocol and default port(80 or 443)
will not show the port, the port is only assigned to default port 443
when the protocol is neither one of them.