Repository navigation
url.parse("http://:::1") is parsed as "http://:1/::" #2929
Description
Activity
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.
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).
- addedurlIssues and PRs related to the legacy built-in url module.Issues and PRs related to the legacy built-in url module.
on Sep 17, 2015 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
aand, filling in the equivalent NodeUrlobject, (and disregarding thatnull,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
hrefproperty. It seems like that really shouldn't get mangled quite the way it does in Node.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
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).
Another result from Chrome:
new URL('http://:::1')=>Uncaught TypeError: Failed to construct 'URL': Invalid URLThat 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 definesurl.parse()? We'd still need a (hopefully thin) wrapper to deal with things like theslashesproperty. 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.)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. ;-)
Closing due to inactivity and an apparent general consensus that the cure may be worse than the disease.
webpack/webpack-dev-server#240 (comment)