doc: clarify url doc

[jasnell]

Indicate that base is ignored if input is absolute.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• documentation is changed or added
• commit message follows commit guidelines

[wir3less]

Please also indicate this is only the case if the Schemas of base and input are different.
If they are the same base will not be ignored.
Also, if Schemas are not Special (as defined by the RFC) base is also not ignored.

[jasnell]

@wir3less ... what are you referring to when you say "Schemas"?

I just went through and confirmed a number of cases:

newURL('http://example.org','http://foo.com')// --> http://example.orgnewURL('foo://bar','foo://baz')// --> foo://barnewURL('foo://bar','http://foo.com')// --> foo://barnewURL('http://foo.com','foo://bar')// --> http://foo.comnewURL('foo://bar','bar://foo')// --> foo://barnewURL('foo:bar','bar:foo')// --> foo:bar

In these cases, regardless of whether the protocol was "special" or the same, the base is ignored.

[Trott]

Micro-nit: Remove the two instances of the as we do not prefix variable names with it generally. I see we do it in the immediately preceding paragraph so I'm fine if you want to ignore this comment, as at least it's consistent in the immediate vicinity. But if you wanted to remove them from that paragraph too, cool by me.

[TimothyGu]

LGTM with @Trott's comment fixed.

[wir3less]

@jasnell this is what I mean, sorry I forgot Slashes play a role here as well:

const{URL}=require('url');console.log(newURL('http://xxx.com','https://google.com').href);// => http://xxx.com/console.log(newURL('https://xxx.com','https://google.com').href);// =>https://xxx.com/console.log(newURL('http:xxx.com','https://google.com').href);// => http://xxx.com/console.log(newURL('https:xxx.com','https://google.com').href);// => https://google.com/xxx.com

[jasnell]

Ah yes, the new URL('http:xxx.com', https://google.com`).href` example is a fun one. The reason this happens is because http:xxx.com is not an absolute URL but does have a protocol. Because the protocols do not match, the input cannot be resolved against the base... that is, you can only resolve a relative URL against a base that uses the same protocol. So what happens in this case is a fallback parsing rule that treats http:xxx.com as http://xxx.com. Again, the way to catch this is to check the origin property of the resulting URL. If the origin of the resulting URL does not match the origin you were expecting from your base, your code should handle that case.

I can add additional wording to the docs to illustrate this case but suggested wording would be helpful.

[wir3less]

Happy to see someone enjoy these as much as I do :)
I agree that the docs should reflect this somehow, maybe the right call here is to provide some examples of these edge cases? Then when the developer has some context, we can advise he checks the origin.
WDYT?

[jasnell]

Yep, examples are good. The one thing we need to be careful of is the fact that the URL standard was written to support many edge cases, it would be quite difficult to adequately cover them all. Let's play with some wording on this particular case but keep that in mind in terms of how we do it.

[wir3less]

How about changing the function signature in the docs to be:

Constructor: new URL(input[, base])#
• input The Absolute or Relative URL to parse. If relative, base is required.
• base | The base URL to resolve against if the input is not absolute.

And adding the following after the first paragraph (before TypeError):

In cases where this function is used with untrusted data, it is advised to validate the origin of the output URL and make sure it falls within the desired domain.

const{URL}=require('url');varmyURL=newURL('http://anotherExample.org/','https://example.org/');// http://anotherexample.org/myURL=newURL('https://anotherExample.org/','https://example.org/');// https://anotherexample.org/myURL=newURL('foo://anotherExample.org/','https://example.org/');// foo://anotherExample.org/myURL=newURL('http:anotherExample.org/','https://example.org/');// http://anotherexample.org/myURL=newURL('https:anotherExample.org/','https://example.org/');// https://example.org/anotherExample.org/myURL=newURL('foo:anotherExample.org/','https://example.org/');// foo:anotherExample.org/

[jasnell]

That's good but I don't think we even need to bring the "untrusted" data into it... the same bit about needing the check the origin applies regardless if there's any chance at all that the input can be an absolute URL or use a different protocol.

[wir3less]

I'm willing to agree here, as long as we have the examples showing some edge cases, I believe developers will get the point.

[jasnell]

@wir3less ... how's it look now?

[jasnell]

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/522/
New CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/523/

[wir3less]

Looks good @jasnell
You have my blessing to merge :)

[lpinca]

Landed in cf48d1d.

[wir3less]

Can someone share the status of this issue?
I was under the impression we're merging this more than a week ago...

[BridgeAR]

@wir3less that is correct. I guess you wonder why the docs are not updated on the website? This will be included in 10.x and likely in the next 9.x. It might also be included in 8.x at some point.

[wir3less]

got you, thanks for the update!