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
feat: Query string params #1449
Changes from 30 commits
2b56a54
7607b90
a4d185e
176cce2
def801f
bf7433a
433a455
5afcdc8
05bc042
52089aa
a89e7aa
06a0093
18744d4
3fdd223
8f6f998
4c711c6
d270947
5d35025
babcf18
2eb2797
a471788
7bb502a
edd2272
fb9f354
ad84f02
86014dd
5e0bf8b
ffc6db2
54c97e5
6f3802e
c3f71f9
63e7f07
5dba6b7
8d2c7d4
ed81bbb
a454d00
79610dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,59 @@ function isBlobLike (object) { | |
) | ||
} | ||
|
||
function isDate (val) { | ||
return toString.call(val) === '[object Date]' | ||
} | ||
|
||
function isObject (val) { | ||
return val !== null && typeof val === 'object' | ||
} | ||
|
||
// this escapes all non-uri friendly characters, but restores : which is commonly used for date formatting | ||
function encode (val) { | ||
return encodeURIComponent(val).replace(/%3A/gi, ':') | ||
} | ||
ronag marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// based on https://github.com/axios/axios/blob/63e559fa609c40a0a460ae5d5a18c3470ffc6c9e/lib/helpers/buildURL.js (MIT license) | ||
function buildURL (url, queryParams) { | ||
if (url.includes('?')) { | ||
throw new Error('Query params cannot be passed when url contains "?" already.') | ||
} | ||
kibertoad marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
ronag marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const parts = [] | ||
for (let [key, val] of Object.entries(queryParams)) { | ||
if (val === null || typeof val === 'undefined') { | ||
continue | ||
} | ||
|
||
if (!Array.isArray(val)) { | ||
val = [val] | ||
} | ||
|
||
for (let v of val) { | ||
if (isDate(v)) { | ||
v = v.toISOString() | ||
kibertoad marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} else if (isObject(v)) { | ||
throw new Error('Passing object as a query param is not supported, please serialize to string up-front') | ||
} | ||
parts.push(encode(key) + '=' + encode(v)) | ||
} | ||
} | ||
|
||
const serializedParams = parts.join('&') | ||
|
||
if (serializedParams) { | ||
const hashmarkIndex = url.indexOf('#') | ||
if (hashmarkIndex !== -1) { | ||
url = url.slice(0, hashmarkIndex) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bit magical... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can drop it, if you like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Throw if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not resolved yet. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry, change was done and test was added, but some garbage left behind |
||
} | ||
|
||
url += '&' + serializedParams | ||
kibertoad marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
return url | ||
} | ||
ronag marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
function parseURL (url) { | ||
if (typeof url === 'string') { | ||
url = new URL(url) | ||
|
@@ -357,5 +410,6 @@ module.exports = { | |
isBuffer, | ||
validateHandler, | ||
getSocketInfo, | ||
isFormDataLike | ||
isFormDataLike, | ||
buildURL | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why restore
:
? I don't understand the use case?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Escaping
:
is going to mess up date format and complicate rather frequent use-case.I'm having second thoughts about escaping anything, TBH, especially since Undici is not supposed to be used from browser. What do you think about removing escaping completely and let user do it if they need to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does got do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we actually need to escape or we might end up with invalid URLs, e.g. if they contain
?
,&
etc...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
encodeURIComponent
is sufficient for now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got is using URLSearchParams, and there are already issues with that: sindresorhus/got#1180
If we go with escaping, I would suggest following axios example instead of got - they were around for much longer, and had all the time in the world to work out the kinks of real world usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand the use case. Is it that some server implementations don't decode query parameters at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer if we just keep it simple and use
encodeURIComponent
. Anything else feels opinionated to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done