Repository navigation
v7.x undocumented backward incompatibility of url.format() #11103
Description
- 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?
Activity
[-]v7.x undocument backward incompatibility of url.format()[/-][+]v7.x undocumented backward incompatibility of url.format()[/+]mscdex commentedon Feb 1, 2017
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 commentedon Feb 1, 2017
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: falsein these cases, the slashes were still prepended. That seems like a bug to me. This fixes that behavior.Now, if
slashesis a false-y value (such asundefined), 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 commentedon Feb 2, 2017
This use case is interesting when looked at in context with our plans to eventually deprecate
url.{format,parse,resolve}. The WHATWGURLinterface do not support empty schemes, nor does the low-level URL serializer./cc @jasnell
joyeecheung commentedon Feb 2, 2017
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 commentedon Mar 23, 2017
Any updates on this?
Trott commentedon Mar 24, 2017
/cc @nodejs/lts for opinions on bugfix vs regression and also whether/how to document in changelogs
addaleax commentedon Mar 24, 2017
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(There’s probably a better text for that, this is just what the issue sounds like to me.)
Trott commentedon Jul 30, 2017
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 commentedon Jul 30, 2017
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.
doc, url: add changelog metadata for url.format
doc, url: add changelog metadata for url.format