-
-
Notifications
You must be signed in to change notification settings - Fork 118
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: support IPv6 during ClientRequest options parsing #489
Conversation
) | ||
}) | ||
|
||
describe('IP v6', () => { |
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.
I recommend we add a separate test file for IPv6 instead of using describe()
.
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.
Sure!
in utils
? ip6.test.ts
?
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.
We only have a couple of tests for IPv6, so how about this?
it('parses "host" in IPv6')
it('parses "host" and "port" in IPv6')
const options: RequestOptions = { | ||
protocol: 'https:', | ||
hostname: '127.0.0.1:1234', |
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.
I'd be extremely careful around changes like this.
Yes, this is technically right—hostname
does not include port. But in practice, I've seen a lot of libraries misuse Node.js. I fear this was one of those cases.
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.
@kettanaito, can you give an example? Because it's invalid and Node throws an error immediately:
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.
That's a good point! Let's remove this old logic and if any issues arise, tackle them separately. I have no notion of supporting hacky tools, and if you use http.request()
incorrectly, you are hacky.
@@ -50,15 +50,6 @@ function getPortByRequestOptions( | |||
return Number(options.port) | |||
} | |||
|
|||
// Extract the port from the hostname. | |||
if (options.hostname != null) { |
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.
Should this be options.host
instead?
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.
host
or hostname
must not include port, so we can't infer the port from it.
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.
🤔 What kind of hackery were we supporting with this.
Ditto, let's remove it as well. Fewer hacks, everyone wins.
return `[${host}]${portString}` | ||
} | ||
if (host) { | ||
if (isRawIPv6Address(host)) { |
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.
@kettanaito Maybe we should use https://nodejs.org/api/net.html#netisipv6input instead?
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 great! Thank you so much, @mikicho 🙏
Released: v0.29.0 🎉This has been released in v0.29.0! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
Following of this comment, I revisited the options parsing rules according to Node.js source and docs, and a few insights:
host
andhostname
are the same, hostname is preferred if both are specified.Domain Name
and doesn't include the port.port
in thehost
, Node refers to it as part of thehost
, and the result is an invalid URL like:http://google.com:1234:80
I also added tests for ipv6.
Last, the current implementation only differs from Node.js in that we return an
Invalid URL
instead of firing a bad request. This is due to the fact that we convert options to URL first. However, I believe this behavior is acceptable.should fix #451