v7.x undocumented backward incompatibility of url.format()

[mrtbld]

• Version: v7.5.0
• Platform: Fedora release 23 (Twenty Three) Linux 4.8.13-100.fc23.x86_64
• Subsystem: url

Latest major release v7.x changes the default value for slashes in url.format() when given objet has a hostname but no protocol. Previously slashes: true wasn't needed in that case, but now it is.

Example:

$ node --version
v6.9.5
$ node
> url = require('url')
> url.format({ hostname: 'foo.example', pathname: '/bar' })
'//foo.example/bar'

$ node --version
v7.5.0
$ node
> url = require('url')
> url.format({ hostname: 'foo.example', pathname: '/bar' })
'foo.example/bar'
> url.format({ hostname: 'foo.example', pathname: '/bar', slashes: true })
'//foo.example/bar'

Generating scheme-relative URLs is useful for a website that support both http and https; this is a common use case IMHO.

I found no mentions of this in the changelog. I believe it's a side effect of commit 336b027 regarding file: URLs (removal of !protocol condition).

Is this a bug or just a changelog oversight? API documentation seems to be in line with this new behavior (since v6.x), is this an undocumented bugfix then?

[mscdex]

Well, it was a semver-major change, so it was released in v7.0.0 and not v6.x (the current major version at the time the commit was landed).

/cc @Trott for more info about the change itself

[Trott]

Thanks for reporting this, @mrtbld!

I'm open to counter-arguments here, but this seems like a bugfix, albeit one perhaps caused by an unintentional side effect (and thus no mention in the changelog).

In v6.x, if you specified slashes: false in these cases, the slashes were still prepended. That seems like a bug to me. This fixes that behavior.

Now, if slashes is a false-y value (such as undefined), you no longer get slashes.

I agree that protocol-relative URLs are an important use case and we don't want break the world over a change in url.format(). And I'm glad this landed as a semver-major.

Like I said, I'm open to counter-arguments here. For one thing, if there's reason to believe that this is causing significant ecosystem breakage and headaches, that certainly overrides my general "I feel like this is a bugfix" sentiment. My impression, though, is that it hasn't caused much trouble during the 3 months that it's been in a supported release. Contrary data (like a pile of issues filed against Express or whatever) totally welcome.

[TimothyGu]

This use case is interesting when looked at in context with our plans to eventually deprecate url.{format,parse,resolve}. The WHATWG URL interface do not support empty schemes, nor does the low-level URL serializer.

/cc @jasnell

[joyeecheung]

Has a CITGM run complained about this breaking change? (I believe CITGM CI doesn't really work at the moment...?)

Also if this behavior is a wonfix, then we should probably mention this in the next change log and the change log of v8.x (don't know who to cc)

[TimothyGu]

Any updates on this?

[Trott]

/cc @nodejs/lts for opinions on bugfix vs regression and also whether/how to document in changelogs

[addaleax]

whether/how to document in changelogs

This could go into a yaml changelog for url.format, I think. I am not 100 % sure I understand the exact conditions in which the behaviour differs, but maybe something like

<!-- YAML
changes:
  - version: v7.0.0
    pr-url: https://github.com/nodejs/node/pull/7234
    description: URLs with a `file:` scheme will now always use the corrent number of slashes. Also, the `slashes` option is supported properly now.
-->

(There’s probably a better text for that, this is just what the issue sounds like to me.)

[Trott]

OK, so: This probably should have been documented in the change log but it also didn't cause widespread heartache. My apologies to those for whom it caused difficulties. I'll try to be more mindful of marking things as notable changes in the future. Not sure there's any action to be taken on this at this point, so I'm going to close. By all means, comment (or re-open if GitHub allows) if you think there's something to be done at this point.

[TimothyGu]

IMO there should be a note (in the form of YAML changelog as suggested by @addaleax) next to url.parse() documenting this change of behavior, so reopening.