url.parse("http://:::1") is parsed as "http://:1/::"

[syranide]

webpack/webpack-dev-server#240 (comment)

console.log(require('url').parse('http://:::1').href) // "http://:1/::"

[Trott]

Literal IPv6 URLs need to be enclosed in [] per RFC 2732.

console.log(require('url').parse('http://[:::1]').href)
// -> http://[::1]/

I'm not so sure it shouldn't throw an error or at least a warning on the example you provide, though.

[syranide]

Yeah, not saying it's correct use of IPv6, but I would expect it to not mess up the parsing at least (i.e. input ~= output).

[Trott]

I believe the two main rules for the url.parse() code are:

• Try to emulate how browsers treat URLs as closely as possible
• OMG do not touch this code if you don't have to because so much of the ecosystem can blow up if an edge case changes!

Focusing on the first rule for now, here's what I get from parse.url('http://:::1');

Url {
  protocol: 'http:',
  slashes: true,
  auth: null,
  host: ':1',
  port: '1',
  hostname: '',
  hash: null,
  search: null,
  query: null,
  pathname: '/::',
  path: '/::',
  href: 'http://:1/::' }

For comparison, I did this in Chrome:

var a = document.createElement('a');
a.href = 'http://:::1';

Then I inspected a and, filling in the equivalent Node Url object, (and disregarding that null, undefined, and empty string are not actually all equivalent), I get:

Url {
  protocol: ':',
  slashes: ¯\_(ツ)_/¯,
  auth: null,
  host: ':0',
  port: '0',
  hostname: '',
  hash: null,
  search: null,
  query: null,
  pathname: '',
  path: null,
  href: 'http://:::1/' }

Given all that, I would say it's a mess either way and (given the second bullet point above) probably not worth "fixing". The exception is the href property. It seems like that really shouldn't get mangled quite the way it does in Node.

[Trott]

Related: For an impressive list of URLs along with how they are parsed by Chrome: http://src.chromium.org/viewvc/chrome/trunk/src/url/url_parse_unittest.cc

[syranide]

It's interesting that it's parsed the same way in chrome... anyway, I'm not really bothered by this myself and while it's not great, as you say more harm can (will) come from trying to fix it than just leaving it as it is.

Leaving it for you to close (your decision).

[targos]

Another result from Chrome: new URL('http://:::1') => Uncaught TypeError: Failed to construct 'URL': Invalid URL

[Trott]

That result makes a lot more sense to me. Do you know if the implementation is in Blink/Chromium code or in v8? I'm guessing it's not in v8 but if I'm wrong, is it absurdly naive of me to ask if it's possible that Node can hook into it and maybe it can be called by url.parse(), allowing us to ditch a lot of the seemingly convoluted JS code that defines url.parse()? We'd still need a (hopefully thin) wrapper to deal with things like the slashes property. But it sure would be nice to offload that stuff to a canonical implementation devised from v8 and/or the browser. I'm guessing I'm not the first person to think of this and that there's an excellent reason it is not done that way. (Such as: It's not in v8.)

[bnoordhuis]

Do you know if the implementation is in Blink/Chromium code or in v8? I'm guessing it's not in v8

It's not V8. ;-)

[Trott]

Closing due to inactivity and an apparent general consensus that the cure may be worse than the disease.