Filter out undefined/null

[jeffijoe]

The following should be equivelant:

urljoin('http://google.com', 'test'); // http://google.com/test
urljoin('http://google.com', 'test', undefined); // http://google.com/test/

Note the trailing slash.

[ArmorDarks]

Few other cases, which produces wrong results:

urljoin(undefined, 'test')
// -> /test, should be test

urljoin('', 'test')
// -> /test, should be test

urljoin(undefined, '/test')
// -> //test, should be test

urljoin('', '/test')
// -> //test, should be test

[ArmorDarks]

Wait. I think it would be more appropriate to throw on null or undefined, while empty strings should be simply ignored (currently they produces /).

[jfromaniello]

I just published v4.0.0, the new behavior will throw an exception like path.join does.

[fernandocanizo]

Hello @jfromaniello , I come to confirm your last comment on #36

I think I've reached a point in this library where every time I merge a PR I break someone else use case.

The change on url-join.js:27:

throw new TypeError('Url must be a string. Received ' + component);

Broke all my end-point constructions where a numeric id is required, i.e.:

urlJoin('http://some.rest.api/books', 3)

I saw that path.join also throws for that use case, so, if you intend to keep url-join analogous to path.join, then please write a big bold uppercase BREAKING CHANGES in the CHANGELOG.md so we are aware before upgrading.

[fernandocanizo]

Reading the original issue again, I'm starting to think this change should be reverted/fixed: maybe we can safely use old behavior on any component that has a .toString() method, or maybe we can still use the old behavior on types we know are safe to convert to string.

Another concern I have is that the objective of the library should be to compose proper urls, so things like...

urljoin(undefined, 'test')
// -> /test, should be test

urljoin('', 'test')
// -> /test, should be test

urljoin(undefined, '/test')
// -> //test, should be test

urljoin('', '/test')
// -> //test, should be test

... throw because don't look like URLs?

Also, the original issue cares about a thing we should not care: a trailing slash. When it comes to URLs, a trailing slash is a good thing, in fact, not having a trailing slash is bad So does it matter if my URLs include a trailing slash?:

One of the most wasteful redirects happens frequently and web developers are generally not aware of it. It occurs when a trailing slash (/) is missing from a URL that should otherwise have one

Haven't seen url-join test cases, so I'm not sure about what to do on empty strings. Surely it must bark an error on undefined, null or NaN, because that's clearly an error.

Finally, this cannot be taken as a rule, but I tried a few sites that use extensions like http://some.site.com/some/article.html and it works perfectly with trailing slash.

So I think we should consider which case produces the lesser evil.

[jfromaniello]

I think we can allow numbers, I am not sure about other things, it might get confusing.

with regards to the trailing slash the problem was the unintended consequence.. the very first example in this issue is:

urljoin('http://google.com', 'test', undefined); // http://google.com/test/

I don't think the user in this case actually want a trailing slash, the problem was that this library was failing silently.

Let's consider a case where the user want to append something to the url and also wants a trailing slash regardless if is the argument segment is null or not.

with the previous behavior you have to do something like this:

> var id;
> urljoin('http://example.com/products', id, '/')
'http://example.com/products//'
> id = '213';
'213'
> urljoin('http://example.com/products', id, '/')
'http://example.com/products/213/'

as you can see in the first example there is a double slash at the end.

with v4 you can do something like this and get the expected result:

> var id;
undefined
> urljoin('http://example.com/products', id || '', '/')
'http://example.com/products/'
> id = '213';
> urljoin('http://example.com/products', id || '', '/')
'http://example.com/products/213/'

I'd like this library to follow POLA (principle of least astonishment).

Regarding the CHANGELOG I think it states clearly here:
https://github.com/jfromaniello/url-join/blob/master/CHANGELOG.md#400---2018-02-02

Ignore empty string arguments and throw an exception for non-string.

[fernandocanizo]

I just made a PR for your consideration. It reinstates numbers as a possible parameter and its test. Also adds some missing primitive data type tests in accordance with your comment:

I think we can allow numbers, I am not sure about other things, it might get confusing.

So we allow strings and numbers and nothing else.

Regarding CHANGELOG.md I noticed you generate it with a script using conventional-changelog so maybe you have no way to include a "BREAKING CHANGES" text automatically. Don't know, maybe by writing it on the commit that inserts the breaking change?

A possible solution would be to edit it before committing, adding necessary clarifications, because while it clearly states the changes done, it's hard for a person skimming through it to see if it's safe to make an upgrade.

To emphasize this I'll describe my current situation: I'm upgrading all dependencies on six months old project, so I have many changes. My workflow is like this:

1. update all packages with minor version change in one go && test app still works without problems
2. then review Changelog or any other documentation about possible changes for each package with a major version update, if I see a "breaking changes" title or text, stop there, review it deeper, update and apply proposed changes. Finally test everything is working fine.
3. next package with major version update

As you could perceive, it's pretty exhausting and big text plus the explicit phrase: "breaking changes" helps a lot to make this faster.