Repository navigation
url.format does not postfix slashes to protocol but doc pretend it should by default #3361
Description
Activity
- addedurlIssues and PRs related to the legacy built-in url module.Issues and PRs related to the legacy built-in url module.
on Oct 14, 2015 Just pasting what I had found here, as I had created a duplicate issue.
If you look at https://github.com/nodejs/node/blob/master/lib/url.js you see:
// only the slashedProtocols get the //. Not mailto:, xmpp:, etc. // unless they had them to begin with. if (this.slashes || (!protocol || slashedProtocol[protocol]) && host !== false) { host = '//' + (host || ''); if (pathname && pathname.charAt(0) !== '/') pathname = '/' + pathname; } else if (!host) { host = ''; }So the documented behaviour only happens when host/hostname are set.
You can check this with the following snippet:var url = require('url'); var unslashedUri = url.parse('/no-slashes'); var slashedUriByHost = url.parse('/slashes-host'); var slashedUriForced = url.parse('/slashes-forced'); unslashedUri.protocol = 'file'; console.log(url.format(unslashedUri)); // file:/no-slashes slashedUriByHost.protocol = 'file'; slashedUriByHost.host = 'localhost'; console.log(url.format(slashedUriByHost)); // file://localhost/slashes-host slashedUriForced.protocol = 'file'; slashedUriForced.slashes = true console.log(url.format(slashedUriForced)); // file:///slashes-forced- added a commit that references this issue
on Dec 2, 2015 Hmm, I'd say this is worth a breaking change as a file uri without
://is invalid in all cases. Output should adhere tofile://host/path, host being''when not defined.- addedsemver-majorPRs that contain breaking changes and should be released in the next major version.PRs that contain breaking changes and should be released in the next major version.
on Dec 3, 2015 To be sure we're ok on the desired end result, this test would currently fail :
'/some/path' : { 'href': '/some/path', 'pathname': '/some/path', 'path': '/some/path', 'host':'', 'hostname':'' }(in
test/parallel/test-url.js)
as host and hostname would benull.
It probably won't be acceptable to have host always be defined to''instead ofnullwhen protocol isnull. That would make the fix easy (init host and hostname to''?) but would have side effects probably well above possible benefits.for example it would cause some trouble for major use cases where we want host to be null, but protocol is context dependant (/index.html in ishttp:while in browser nav it'sfile:).More sensible solution would be to have something specific to protocols that don't have a hostname but still want slashes. A bit like the way
javascript:is handled.Or did I miss something?
Hmm, that's an unexpected failure. Unfortunately, I'm not really familiar with that code. I agree that a failure like this is unacceptable. By the way, we have #2303 which is pretty much a rewrite of the module for perf reasons, which also contains a few breaking changes. If a fix for this turns out to be too complicated, maybe it's better to incorporate it there.
Reopened because 2a29b70 does not take care of the issue, it just documents the current behaviour.
- added a commit that references this issue
on Dec 8, 2015 - added a commit that references this issue
on Dec 29, 2015 - added a commit that references this issue
on Jan 19, 2016 - added a commit that references this issue
on Apr 2, 2016 https://url.spec.whatwg.org/#url-syntax indicates that all special schemes (
ftp,file,gopher,http,https,ws,wss) must be followed by a:and scheme-relative URL, which starts with//.So it seems that any URL generated with those schemes specified should include
//. PR coming shortly, but I don't know how receptive people are going to be to it.- added a commit that references this issue
on Jun 16, 2016
Quoting the doc :
But a quick test (using nodejs V4.1) show the problem:
Output : file:/home/user. While the expected output is : file:///home/user
Setting
uri.slashes = truemake it format the correct URL.I think it's not worth it to modify the module itself as it's stable. However the doc should be updated to reflect this behaviour.
Should I submit a PR reflecting this?