Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
tivac opened this issue Apr 6, 2016 · 9 comments
Closed

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

tivac opened this issue Apr 6, 2016 · 9 comments

Comments

@tivac
Copy link

tivac commented Apr 6, 2016

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.

@nvartolomei
Copy link

nvartolomei commented May 23, 2016

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

@jfromaniello
Copy link
Owner

seems like a bug

@tivac
Copy link
Author

tivac commented May 23, 2016

@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.

@nvartolomei
Copy link

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 :)

@eugenet8k
Copy link

eugenet8k commented Jul 6, 2016

Also:

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

and

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

matz3 added a commit to SAP-archive/grunt-openui5 that referenced this issue Nov 30, 2016
The "url-join" package behaves incorrect in the currently latest
version.
(See jfromaniello/url-join#16)
Therefore it's replaced with "urljoin" which works as expected.
@ArmorDarks
Copy link

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.

@intijk
Copy link

intijk commented Dec 10, 2017

Is this really fixed, why it is still buggy?

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

Is this project still under maintenance?

@ArmorDarks
Copy link

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

@jfromaniello
Copy link
Owner

These cases are already handled in the latest version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants