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

url: make urlToOptions() handle IPv6 literals #19720

Closed
wants to merge 1 commit into from

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Apr 1, 2018

Strip the enclosing square brackets from the parsed hostname.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Strip the enclosing square brackets from the parsed hostname.
@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Apr 1, 2018
@lpinca
Copy link
Member Author

lpinca commented Apr 1, 2018

@lpinca
Copy link
Member Author

lpinca commented Apr 6, 2018

@@ -1312,7 +1312,9 @@ function domainToUnicode(domain) {
function urlToOptions(url) {
var options = {
protocol: url.protocol,
hostname: url.hostname,
hostname: url.hostname.startsWith('[') ?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: from a performance standpoint it would be better to use charCodeAt.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to be consistent with

if (hostHeader.startsWith('[')) {

Both can be changed in a follow up PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think this is a performance-relevant function anyway. The actual HTTP roundtrip will take much longer.

@lpinca
Copy link
Member Author

lpinca commented Apr 10, 2018

Landed in fa2d43b.

@lpinca lpinca closed this Apr 10, 2018
lpinca added a commit that referenced this pull request Apr 10, 2018
Strip the enclosing square brackets from the parsed hostname.

PR-URL: #19720
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@lpinca lpinca deleted the fix/url-to-options branch April 10, 2018 07:07
targos pushed a commit that referenced this pull request Apr 12, 2018
Strip the enclosing square brackets from the parsed hostname.

PR-URL: #19720
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants