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

Duplicate slashes "//" in urls when a part is empty #36

Closed
crccheck opened this issue Feb 1, 2018 · 6 comments
Closed

Duplicate slashes "//" in urls when a part is empty #36

crccheck opened this issue Feb 1, 2018 · 6 comments

Comments

@crccheck
Copy link

crccheck commented Feb 1, 2018

url-join version 3.0.0

> urlJoin('path/', '', '/test')
'path/test'  # expected
'path//test' # actual

> urlJoin('path', '', /test')
'path/test'  # expected
'path//test' # actual

> urlJoin('path', null, /test')
'path/test'  # expected
'path//test' # actual

In more detail, I've got an array of bits of a url. Sometimes, parts are optional, so I set them to '' and assume the url joiner will throw away the empty bits

@crccheck
Copy link
Author

crccheck commented Feb 1, 2018

ugh, submitted before I wrote the issue

@crccheck crccheck changed the title Duplicate urls Duplicate slashes "//" in urls when a part is empty Feb 1, 2018
@crccheck
Copy link
Author

crccheck commented Feb 1, 2018

as prior art, this is the same behavior as Node's built in path.join:

$ node
> const path = require('path')
undefined
> path.join('hi')
'hi'
> path.join('hi', 'there')
'hi/there'
> path.join('hi', '', 'there')
'hi/there'

@jfromaniello
Copy link
Owner

Thanks for reporting this. I just published v4 addressing this the same way that path.join does.

@shellscape
Copy link

@jfromaniello this could have been documented much better than it is in the changelog. This also effects urlJoin('', 'foo.js');. Previously the return value was /foo.js, in version 4, it's foo.js. Folks that aren't aware of the nuances of path.join will be thrown off by this because of prior expected behavior of the module.

@crccheck
Copy link
Author

ooh, I didn't even think of that use case. I'll try to keep that in mind in the future too. For reference, here's Node's path.join with some variations of that case:

> path.join('', 'foo.js')
'foo.js'
> path.join('', '/foo.js')
'/foo.js'
> path.join('/', 'foo.js')
'/foo.js'
> path.join('/', '/foo.js')
'/foo.js'

@jfromaniello
Copy link
Owner

I think I've reached a point in this library where every time I merge a PR I break someone else use case. I am trying to figure a way, I think to begin with we need a lot more of tests and specifications.

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

3 participants