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

perf: use isIPv6 for checking if hostname is isIPv6 #3466

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Aug 16, 2024

trivial change. isIP will first run isIPv4 and then isIPv6. Also both are basically calling regexes on node. So by calling isIPv6 directly we improve the performance.

@ronag
Copy link
Member

ronag commented Aug 16, 2024

But this will fail on ipv4 addresses now?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Aug 16, 2024

Nope. It can not be a IP v4. This is basically "unpacking" a IPv6 from the bracket notation. Like http://[::1]:3000. hostname is then [::1] but needs to be ::1. So this is detecting the wrapping with brackets by checking the if it is starting with a bracket etc..

An IPv4 would not be valid.

See RFC 3986, section 3.2.2.

   A host identified by an Internet Protocol literal address, version 6
   [RFC3513] or later, is distinguished by enclosing the IP literal
   within square brackets ("[" and "]").  This is the only place where
   square bracket characters are allowed in the URI syntax.  In
   anticipation of future, as-yet-undefined IP literal address formats,
   an implementation may use an optional version flag to indicate such a
   format explicitly rather than rely on heuristic determination.

@ronag ronag merged commit 5e50e44 into nodejs:main Aug 16, 2024
32 checks passed
@Uzlopak Uzlopak deleted the is-ip-v6 branch August 16, 2024 14:42
@github-actions github-actions bot mentioned this pull request Dec 3, 2024
@github-actions github-actions bot mentioned this pull request Dec 16, 2024
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