Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions test/url.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,25 @@ tape('url', (t) => {
const arg = new Map(Object.entries({ x: 10, b: 20 }))
check(url`https://example.org/buz/?d&${arg}`, 'https://example.org/buz/?d&x=10&b=20')
})
t.doesNotThrow(() => {
const baseUrl = new URL('https://example.org')
check(url`${baseUrl}/foo`, 'https://example.org/foo')
})
t.doesNotThrow(() => {
// baseURL ends with / and endpoint does too.
const baseUrl = new URL('https://example.org/')
check(url`${baseUrl}/foo`, 'https://example.org/foo')
})
t.doesNotThrow(() => {
// baseURL ends with / but endpoint doesn't.
const baseUrl = new URL('https://example.org/')
check(url`${baseUrl}foo`, 'https://example.org/foo')
})
t.doesNotThrow(() => {
// baseURL doesn't ends with / and endpoint doesn't.
const baseUrl = new URL('https://example.org')
check(url`${baseUrl}foo`, 'https://example.org/foo')
})

t.end()
})
Expand Down
25 changes: 24 additions & 1 deletion url.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,30 @@ function url(strings, ...args) {

return encodeComponent(raw)
})
const res = [strings[0], ...escaped.flatMap((arg, i) => [arg, strings[i + 1]])].join('')

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember why we added url, maybe @sparten11740 or @ChALkeR knows better

// Handle the case where a URL object is used as base and creates double slashes
let res

if (base && escaped.length > 0) {
const parts = [strings[0]]

for (const [i, escapedArg] of escaped.entries()) {
parts.push(escapedArg)
let nextString = strings[i + 1]

// If the previous part ends with '/' and next string starts with '/', remove the duplicate
if (parts[parts.length - 1].endsWith('/') && nextString.startsWith('/')) {
Comment thread
garana-exodus marked this conversation as resolved.
nextString = nextString.slice(1)
}

parts.push(nextString)
}

res = parts.join('')
} else {
res = [strings[0], ...escaped.flatMap((arg, i) => [arg, strings[i + 1]])].join('')
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the impl looks too complex
will review separately, don't land without a review

@federico-po federico-po Jul 7, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@ChALkeR ChALkeR Jul 7, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update the clients to change it. Although, we should still land this fix or remove url in favor of standard URL

if (base) validateBase(base, res)
const url = new URL(res)
if (String(url) !== res) throw new Error('Unexpected URL produced!') // e.g. .. which get resolved
Expand Down