fix: remove doubled slash when using URL instance #35
Conversation
| @@ -96,7 +96,30 @@ function url(strings, ...args) { | |||
|
|
|||
| return encodeComponent(raw) | |||
| }) | |||
| const res = [strings[0], ...escaped.flatMap((arg, i) => [arg, strings[i + 1]])].join('') | |||
|
|
|||
There was a problem hiding this comment.
Perhaps this has already been discussed: why not use the native URL object? It should account for // in the path.
new URL('/path', 'https://exodus.com/').toString()There was a problem hiding this comment.
I can't remember why we added url, maybe @sparten11740 or @ChALkeR knows better
| } else { | ||
| res = [strings[0], ...escaped.flatMap((arg, i) => [arg, strings[i + 1]])].join('') | ||
| } | ||
|
|
There was a problem hiding this comment.
the impl looks too complex
will review separately, don't land without a review
There was a problem hiding this comment.
I echo's Gonzalo's question above: why do we need to use url? couldn't we just use URL? can't remember when it was asked to use (probably in a PR review)
There was a problem hiding this comment.
couldn't we just use URL
You can use URL. new URL('/foo/bar', 'https://example.com') is perfectly fine.
Just don't do something like https://example.com/${unescaped arg}
There was a problem hiding this comment.
I'll update the clients to change it. Although, we should still land this fix or remove url in favor of standard URL
The test I added fails in master since the url is
https://example.org//foo.The doubled
//ad the end makes fetch fails.(note: I used AI to fix the issue, so please evaluate if anything else is needed)