URI.js icon indicating copy to clipboard operation
URI.js copied to clipboard

`joinPaths` expectations

Open ArmorDarks opened this issue 8 years ago • 5 comments

I wonder about this example from documentation:

URI.joinPaths('', 'a', '');
// returns URI("/a/")

why does it add leading slash?

It seems to be unexpected behavior, since you're like merging "empty" segment with next segment, and there is nothing that indicates that url have to receive leading slash.

Am I missing something?

ArmorDarks avatar Jul 11 '17 11:07 ArmorDarks

The empty segments suggest leading and trailing slashes, as those could otherwise not be added without introducing an options object { leadingSlash: true, trailingSlash: true }. Yes, that may have been the better function signature, but it's not the one we've got right now.

rodneyrehm avatar Jul 11 '17 11:07 rodneyrehm

Shouldn't leading and trailing slashes be explicit, like so?

URI.joinPaths('/', 'a', '/');

This will allow to avoid introduction of such options.

ArmorDarks avatar Jul 11 '17 11:07 ArmorDarks

Okey, I've renamed this issue to be more generic.

I've wrote some my own tests, and I'm not sure that output is always expected.

Here is full lists of unexpected results I was able to find:

import URI from 'urijs'

// single slash:
URI.joinPaths('/') // expected '/', result ''
// multiple slashes:
URI.joinPaths('/', '/') // expected '/', result ''
// multiple slashes with second one double:
URI.joinPaths('/', '//') // expected '/', result ''
// relative protocol with slash:
URI.joinPaths('//', '/') // expected '//', result ''
// relative protocol with double slashes:
URI.joinPaths('//', '//') // expected '//', result ''

// single item with prepended relative protocol:
URI.joinPaths('//', 'single') // expected '//single', result '/single'
// single item having leading slash with prepended relative protocol:
URI.joinPaths('//', '/leadingSlash') // expected '//leadingSlash', result '/leadingSlash'

// single item having dot and prepended relative protocol:
URI.joinPaths('//', '1.0') // expected '//1.0', result '/1.0'

// first item having leading double slash:
URI.joinPaths('//leadingSlash', 'secondItem') // expected '//leadingSlash/secondItem', result '/secondItem'
// second item having leading double slash:
URI.joinPaths('firstItem', '//leadingSlash') // expected 'firstItem/leadingSlash', result 'firstItem/'
// second item leading and trailing double slash:
URI.joinPaths('firstItem', '//leadinTrailingSlash/') // expected 'firstItem/leadinTrailingSlash/', result 'firstItem/'
// both items having leading and trailing double slashes:
URI.joinPaths('//leadinTrailingSlash//', '//leadinTrailingSlash//') // expected '//leadinTrailingSlash/leadinTrailingSlash/', result ''

// both items having leading and trailing slashes and intermediate double slash:
URI.joinPaths('/item/', '//', '//second/') // expected '/item/second/', result '/item/'
// leading empty string:
URI.joinPaths('', 'second') // expected 'second', result '/second'
// leading double empty string:
URI.joinPaths('', '', 'second') // expected 'second', result '/second'
// trailing empty string:
URI.joinPaths('first', '') // expected 'first', result 'first/'
// trailing double empty string:
URI.joinPaths('first', '', '') // expected 'first', result 'first/'
// leading empty string and second item havging leading double slash:
URI.joinPaths('', '//second') // expected '/second', result ''
// empty string with prepended relative protocol:
URI.joinPaths('//', '') // expected '//', result ''
// empty string with prepended slash:
URI.joinPaths('/', '') // expected '/', result ''
// empty string with appended double slash:
URI.joinPaths('', '//') // expected '/', result ''
// empty string with appended slash:
URI.joinPaths('', '/') // expected '/', result ''

My main concerns:

  • Already mentioned empty strings which produces unexpected leading and trailing slashes.

    In my opinion should be handled with explicit slashes rather that unobvious behavior

  • Merging of slashes only for some reason produces empty ``, while I'd say it more logical to say that such urls referencing document root, thus expected result is /

  • Any encounters of leading and trailing // (kinda relative protocol) results in unexpected results

  • Intermediate // produces even stranger results

ArmorDarks avatar Jul 11 '17 16:07 ArmorDarks

// second item having leading double slash:
URI.joinPaths('firstItem', '//leadingSlash') // expected 'firstItem/leadingSlash', result 'firstItem/'

looks like a bug until you realize that each argument is considered an URL, parsing //leadingSlash to { host: 'leadingSlash', path: '' }

rodneyrehm avatar Jul 11 '17 19:07 rodneyrehm

Thanks for reply!

see URI.joinPaths()

yeap, I saw that some things were made on purpose, but I wasn't sure

paths don't have a protocol

well, yes, I realized that library implies other methods to add hostname to paths and they seems to be reasonable. Though, it would be handy to write sometimes something like:

joinPaths('https://domain.com/', 'path')

and receive https://domain.com/path) instead of path.

looks like a bug until you realize that each argument is considered an URL, parsing //leadingSlash to { host: 'leadingSlash', path: '' }

Sounds reasonable, but from other perspective turns out to be very dangerous: happens to leave somewhere in the middle double slash, and it will eat good portion of your path...


And I'm still concerned about those two:

  • Slashes joining:

    // single slash:
    URI.joinPaths('/') // expected '/', result ''
    // multiple slashes:
    URI.joinPaths('/', '/') // expected '/', result ''
    // multiple slashes with second one double:
    URI.joinPaths('/', '//') // expected '/', result ''
    // relative protocol with slash:
    URI.joinPaths('//', '/') // expected '//', result ''
    // relative protocol with double slashes:
    URI.joinPaths('//', '//') // expected '//', result ''
    

    I don't see why they return empty string. In most cases we've provided slash, so it should return at least one slash, because most likely we wanted to receive root-relative path, but it gives as document-relative path.

  • Empty strings joining

    // leading empty string:
    URI.joinPaths('', 'second') // expected 'second', result '/second'
    // leading double empty string:
    URI.joinPaths('', '', 'second') // expected 'second', result '/second'
    // trailing empty string:
    URI.joinPaths('first', '') // expected 'first', result 'first/'
    // trailing double empty string:
    URI.joinPaths('first', '', '') // expected 'first', result 'first/'
    

    As I already mentioned, it seems to be quite magic appereance of slash in place of empty path. I think if someone wanted to ensure that leading or trailing slashes are always in place, he'd be explicit and specify leading and trailing slashes instead, like so:

    URI.joinPaths('/', 'second') // expected '/second'
    URI.joinPaths('first', '/') // expected 'first/'
    

    Not to mention that it isn't consistent with joining of slashes and empty strings, where it produces empty string (thus, effectively making path document-relative where it intended to be root-relative):

    // leading empty string and second item havging leading double slash:
    URI.joinPaths('/', '') // expected '/', result ''
    URI.joinPaths('', '/') // expected '/', result ''
    

    Though, in last example I'd rather expect following results:

    // leading empty string and second item havging leading double slash:
    URI.joinPaths('/', '') // expected '/'
    URI.joinPaths('', '/') // expected ''
    

    Because leading '' most likely implies document-relative path.

ArmorDarks avatar Jul 13 '17 19:07 ArmorDarks