jfromaniello/url-join

v1.1.0 doesn't de-dupe "/" in relative URLs

tivac opened this issue · 9 comments

tivac commented

It looks like the / deduping regex changed after v0.0.1

// v0.0.1
.replace(/[\/]+/g, '/')

// v1.1.0
.replace(/([^:\s])\/+/g, '$1/')

which means that relative URLs now don't correctly dedupe leading repeat /.

// v0.0.1
> join("/", "/");
> "/"

// v1.1.0
> join("/", "/");
> "//"

I can work around it in my own code but if this is only intended to support absolute URLs that may be worth calling out.

Alternatively an option (added via the system introduced in #14) to specify if the URL is intended to be relative and use the older regex may be worthwhile. Thoughts? I can send a PR if you'd like.

Be careful with this, not to break protocol relative urls!

seems like a bug

tivac commented

@nvartolomei That's why I specifically called out a potential solution being adding a way to switch between relative/absolute URL mode, otherwise this seems like a really rough problem to solve.

What if instead when calling join it is checked that slashes are passed as separate arguments then remove them and leave leading double slashes only if are passed in a single argument :) This implementation may be interesting :)

Also:

> join("", "/some/url");
> "//some/url"

and

> join("/", "someurl");
> "//someurl"

That's definitely works wrong... Any plans to fix this? @jfromaniello?

I think that relative protocol should be supported only when it's passed explicitly.

This

urljoin('/', '/some')

should return /some

And this

urljoin('//', '/some')

should return //some, since we passed in relative protocol explicitly.

Library shouldn't make any assumptions.

Is this really fixed, why it is still buggy?

urljoin('/', '/some');
gives //some

Is this project still under maintenance?

We've switched to more reliable URI.js, but for frontend solutions it would be too big in current state...

These cases are already handled in the latest version.