url.format does not postfix slashes to protocol but doc pretend it should by default

[sdumetz]

Quoting the doc :

protocol is treated the same with or without the trailing : (colon).
The protocols http, https, ftp, gopher, file will be postfixed with :// (colon-slash-slash).
All other protocols mailto, xmpp, aim, sftp, foo, etc will be postfixed with : (colon).
slashes set to true if the protocol requires :// (colon-slash-slash)
Only needs to be set for protocols not previously listed as requiring slashes, such as mongodb://localhost:8000/.

But a quick test (using nodejs V4.1) show the problem:

url = require("url");
uri = url.parse("/home/user");
uri.protocol = "file";
console.log(url.format(uri));

Output : file:/home/user. While the expected output is : file:///home/user

Setting uri.slashes = true make 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?

[claudiorodriguez]

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

[silverwind]

Hmm, I'd say this is worth a breaking change as a file uri without :// is invalid in all cases. Output should adhere to file://host/path, host being '' when not defined.

[sdumetz]

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 be null.
It probably won't be acceptable to have host always be defined to ''instead of null when protocol is null. 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 is http: while in browser nav it's file:).

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?

[silverwind]

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.

[silverwind]

Reopened because 2a29b70 does not take care of the issue, it just documents the current behaviour.

[Trott]

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.