lib: fix urlObject parameter name in url.format

[leggiero]

Fixed typo in error message to output the right parameter name url instead of objUrl.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows [commit guidelines]

Affected core subsystem(s)

lib

[lpinca]

Maybe the error message was referring to the urlObject in the documentation but that is still inconsistent.

[Trott]

I think the error message is correct as is. It's referring to the urlObj parameter of url.format(). If anything, it should be changed to say that it needs to be a string or an object, rather than just an object. And maybe a comment could be added to the code explaining why it says urlObj rather than obj. https://nodejs.org/api/url.html#url_url_format_urlobject

[Trott]

I think the error message is correct as is. It's referring to the urlObj parameter of url.format(). If anything, it should be changed to say that it needs to be a string or an object, rather than just an object. And maybe a comment could be added to the code explaining why it says urlObj rather than obj. https://nodejs.org/api/url.html#url_url_format_urlobject

Another possibility is to update the code itself so that the function signature is urlObj rather than obj so the error message is correct. I'm less enthusiastic about changing the documentation to say obj because it can be a string too. urlObj is misleading enough. :-D

[Trott]

The existing comments in that function are confusing (to me, at least0, so there's an opportunity there too...

[leggiero]

PR updated with urlObject parameter name and rebased with master, now there is no conflicts)

Guys, looks like there is another inconsistence here:

The function that we are changing: function urlFormat(obj, options) { from the actual code is exported here as:

module.exports = {
  ...
  // Original API
  format: urlFormat
  ...
}

According with latest and LTS docs, the signature of this method is url.format(urlObject) ... so, other than the wrong parameter name in the actual code (obj) and the wrong name in the error message (urlObj) a new options parameter as been introduced in the actual code and is not documented. This is a legacy API, should not add new parameters.

PS: To make things more confusing: from the v6.2.1 docs onwards, the signature was changed to the actual url.format(urlObject) ... you can check previous docs, like v6.2.0 and see that the signature was url.format(urlObj) :-0

1. Regarding the parameter name (and error message), in my opinion should be changed to urlObject to reflect v6.2.1+ docs (that includes latest and LTS versions).

2. What about this new non-documented options parameter?

[Trott]

@leggiero The additional parameter was added by @jasnell in c5e9654. Documentation was added in 0d9ea4f.

Those commits first appeared in 8.0.0.

The options object is not in any LTS releases at this time and is documented in the (non-LTS) releases where it is.

[Trott]

LGTM if CI is green.

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/8936/

Thanks for the contribution! 🎉

[Trott]

There's currently discussion as to whether or not message text changes to these newer types of errors are considered breaking changes or not. I have no strong opinion about that, but am marking this semver-major defensively until that question is resolved (hopefully later this week). (@cjihrig had already marked it semver-major but I removed it in error, so now I'm adding it back with an explanation.)

[Trott]

@leggiero CI is reporting one or more lint errors. Can you run make jslint (or vcbuild jslint if on Windows) and see what if you can sort out what the problem is? From a quick look, it looks like you might have a line longer than 80 characters.

[leggiero]

@Trott lint error solved.

PS: would be nice to add the make jslint checklist entry in template PR message.

[TimothyGu]

@leggiero make jslint is (or should be) implied in the make test command.

[Trott]

PS: would be nice to add the make jslint checklist entry in template PR message.

If you run make test as described in the PR template message, it runs make lint after the tests pass. (It won't bother if tests fail.) make lint runs both the JS linter and the C++ linter.

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/8971/

[Trott]

Landed in 8520e6f.
Thanks for the contribution! 🎉