url: add join method

[pdilyard]

Allows url paths to be combined with similar semantics to
path.join.

Usage:

url.join('http://example.com', 'path', '/to', 'route');

The url module was missing a method to join several different
parts of a path together into one normalized url string. Until
now, the resolve method has been the closest thing, but only one
path could be passed to append to the base url.

This functionality works by joining the keeping the source part of
the url as-is, combining the rest of the arguments with posix.join,
and then calling url.resolve with the source and combined path.

[thefourtheye]

It would be better if the corresponding doc changes are also done in this PR.

[pdilyard]

@thefourtheye Good point, added in 347bd4c

[Trott]

Running make jslint on your code changes flags a style issue on this line. (Style in Node core would be function(joinTest) without any spaces.)

[Trott]

For reference, userland modules that already implement this functionality:

• https://www.npmjs.com/package/url-join
• https://www.npmjs.com/package/urljoin
• https://www.npmjs.com/package/join-path

[Trott]

What's the use case for this? Not trying to be discouraging, but introducing more API surface in a stable core module would require a pretty rock-solid justification. Is there a reason the userland implementations are insufficient?

[Trott]

This disparity seems like a bug in the implementation:

url.resolve('http://example.com/foo', ''); // result: 'http://example.com/foo'
url.join('http://example.com/foo', '');    // result: 'http://example.com/'

It seems that url.resolve() and url.join() should return the same result if they are given the same two parameters (which is true for the three npm modules mentioned above).

[pdilyard]

@Trott I don't think the userland implementations are insufficient, other than the fact that they require another dependency. I ran into a use-case for this functionality the other day, and instead of adding those packages, I ended up using path.join and then url.resolve. I figured this functionality might be nice to have, but I do understand the point about API surface in a stable module.

If you think this is something that should be added, let me know and I'll fix the bugs and style issues. If not, that's ok too 😄

[pdilyard]

@Trott Any update on this or shall I close?

[Trott]

I'm kind of -0 on this. I wouldn't stop another more enthusiastic collaborator from landing it (although it would take two enthusiastic collaborators, really, because it would need at least one LGTM and either by convention or protocol, people don't land non-trivial changes without an LGTM from someone else).

But if there is only one overarching philosophy for which there is broad agreement among the collaborators, it would be that Node core should be small and that problems that can be solved in userland should be done in userland. (Some folks on the HTTP working group openly wish the http module could be moved out of core and into userland!)

Since @thefourtheye and @ChALkeR have taken some small actions on this PR, I'd be curious if their opinions lined up with mine or not.

[thefourtheye]

I would like to hear from @domenic

[domenic]

I don't think this generally makes sense.

• The idea of "joining" URLs is not well defined in general. The examples in the docs don't explain how cases like join("https://example.com/foo?bar", "baz") or join("https://example.com/foo#bar", "baz") are supposed to work. Not to mention more pathological cases like join("https://example.com/foo?", "#baz").
• It's not even clear what the second argument even is. Absolute URLs are obviously not allowed, so "url.join" clearly does not mean "join these two URLs". (What happens if you try?) Relative URLs don't make too much sense either: in join("https://example.com/foo/bar", "/baz"), "/baz" is clearly not being interpreted as a relative URL, since otherwise the result would be "https://example.com/baz". But then again, ..s are interpreted as in relative URLs for some reason. It's some kind of strange in-between...

What you really want is probably some kind of API specifically designed for the manipulation of a URL object's pathname component. .split("/") and .join("/") get you 90% of the way there, but maybe you want to collapse ..s too. Maybe require("path").posix would do the trick, actually. In any case, it's pretty specialized, and unlikely to make sense in core.

[reqshark]

FWIW, i agree with @domenic

join() was already claimed by Array.prototype a long time ago for this type of string concatenation, so any use of a method by that name in the context of strings that doesn't do exactly what [].join('') does would be unfortunate.

the js standard is clear, http://www.ecma-international.org/ecma-262/6.0/#sec-array.prototype.join join() only takes one argument: a separator for doing string concatenation, so passing lots of string arguments to join() is unorthodox and not what we're used to

[pdilyard]

Thanks for the feedback everyone.