jfromaniello/url-join

should change first & to ?

koheiiwamura opened this issue · 8 comments

koheiiwamura commented

if we use urljoin with parameter that ("http://example.com", "a", "&foo=123", "&bar=456"), it returned http://example.com/a&foo=123&bar=456, but it should be http://example.com/a?foo=123&bar=456

koheiiwamura commented

@jfromaniello

my idea is correct?

    // remove trailing slash before parameters or hash
    str = str.replace(/\/(\?|&|#[^!])/g, '$1');
    // replace & to ? for first & before ?
    var str = str.replace("&", "?");
    // replace ? in parameters with &
    var parts = str.split('?');
    str = parts.shift() + (parts.length > 0 ? '?': '') + parts.join('&');
jonkoops commented

I'm not sure if we want to resolve this issue as it is expected that parameters that are provided in a valid matter. Perhaps this library is doing too much and should focus instead on joining only the path without any additions.

I've created a PR to resolve this issue (#86), but let's discuss it first. @jfromaniello what do you think?

andrew-pyle commented

@jonkoops I believe that you are correct that the library is doing too much — I only need path part merging, like you mention.

Perhaps this library is doing too much and should focus instead on joining only the path without any additions.

I consider the standard APIs URL and URLSearchParams capable of most things necessary.

  • Join origin to pathname. (Note: URL() deduplicates / between origin and pathname, but does not merge pathname parts)
let url = new URL("/wiki/Early_history_of_the_IRT_subway", "https://wikipedia.org/")
url.href
// => "https://wikipedia.org/wiki/Early_history_of_the_IRT_subway"
  • Set query params on a URL
let url = new URL("https://example.com/")
url.search = new URLSearchParams([ ["foo", 123], ["bar", 456] ])
url.href 
// => "https://example.com/?foo=123&bar=456"
  • Add query params to a URL
let url = new URL("https://example.com/")
url.searchParams.set('foo', 123) // to overwrite existing
url.searchParams.append('bar', 456) // to add without overwriting
url.href 
// => "https://example.com/?foo=123&bar=456"
  • Set the url hash
let url = new URL("https://example.com/")
url.hash = "heading-1"
url.href
// => "https://example.com/#heading-1"

But critically, URL cannot join path parts together! I need that!

I want to join path parts together into a pathname without worrying about leading/trailing/duplicating slashes (/)!

  • Merge path parts
// Multiple part arguments unsupported
let url = new URL("/wiki/", "/Early_history_of_the_IRT_subway", "https://wikipedia.org/")
// => Uncaught TypeError: Invalid URL: '/wiki/' with base '/Early_history_of_the_IRT_subway'
// Concatenate path parts has double slash
let url = new URL("/wiki/" + "/Early_history_of_the_IRT_subway", "https://wikipedia.org/")
url.href
// => "https://wikipedia.org/wiki//Early_history_of_the_IRT_subway"
// ______________________________^ double slash

Proposal: URLPathname

@jonkoops Could we spin off the parts of this library which only does URL path joining into a single pathname? Maybe something like URLPathname?

// No duplicated slashes
let url = new URL("https://wikipedia.org/")
url.pathname = new URLPathname("/wiki/", "/Early_history_of_the_IRT_subway");
url.href
// => = "https://wikipedia.org/wiki/Early_history_of_the_IRT_subway"

Or

// No duplicated slashes
let url = new URL(
    new URLPathname("/wiki/", "/Early_history_of_the_IRT_subway"),
    "https://wikipedia.org/"
)
url.href
// => = "https://wikipedia.org/wiki/Early_history_of_the_IRT_subway"

It could work together with the URL and URLSearchParams APIs.

jonkoops commented

Yeah, I actually totally agree with your assessment @andrew-pyle. I think this library was created at a time that the standard URL API did not exist. However, I don't want this to be a big breaking change right away, since the last version that was released was already a major with nothing but breaking changes.

Perhaps a good start would be to update the README to direct users to more standardized APIs, rather than using url-join for all use-cases. And only document the more sane use case that is intended for just the paths.

andrew-pyle commented

@jonkoops Your analysis of url-join’s history and your reticence to make a breaking change both make sense to me.

Would you like a Pull Request to update the README? Or if you’d like to make the change yourself, feel free to use my previous comments verbatim (or with such changes as you see fit).

Alternatively, what do you think of adding another (named) export to the package for joining URL pathname parts only? It could be tree-shaken out if unused, I suppose. That could be a path forward for opt-in usage and eventual deprecation of the current, default export. I have an unpublished library called URLPathname with the interface shown above. I’d be glad to contribute it to this package.

jonkoops commented

Would you like a Pull Request to update the README? Or if you’d like to make the change yourself, feel free to use my previous comments verbatim (or with such changes as you see fit).

Yes, feel free to open up a pull request with some suggestions. I'll make time to review and do some work on it.

Alternatively, what do you think of adding another (named) export to the package for joining URL pathname parts only?

I think what we should do is try to direct people towards the URL API as much as possible. I think further extension of the existing API should be kept to a minimum. What we could do at some point is to start logging deprecation warnings if users attempt to use anything but paths.

jonkoops commented

I went ahead and merged my fix for this, I think it's fine to support this use-case for now, even if the input is kind of invalid, we'll just do a 'best effort' approach to fix it up.