feat: Query string params #1449
Conversation
…8-params � Conflicts: � README.md � index.js � lib/core/client.js � test/client-stream.js � test/client.js � test/headers-as-array.js � test/socket-handle-error.js � types/client.d.ts
Codecov Report
@@ Coverage Diff @@
## main #1449 +/- ##
==========================================
+ Coverage 94.37% 94.42% +0.05%
==========================================
Files 49 49
Lines 4231 4272 +41
==========================================
+ Hits 3993 4034 +41
Misses 238 238
Continue to review full report at Codecov.
|
|
Also needs docs. |
|
@ronag Documentation added, sorry. Slipped through the cracks while I was resolving conflicts with main, documentation was heavily restructured since then. |
|
Failed test seems to be flaky. |
|
OK, there seems to be very specific error happening on Node 16 on Windows. Super fun. Will investigate. |
Co-authored-by: Robert Nagy <ronagy@icloud.com>
|
@ronag This is ready for rereview. Please let me know if you want additional tests for any cases. |
Co-authored-by: Robert Nagy <ronagy@icloud.com>
lib/core/util.js
Outdated
|
|
||
| // 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, ':') |
There was a problem hiding this comment.
Why restore :? I don't understand the use case?
There was a problem hiding this comment.
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.
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.
I think encodeURIComponent is sufficient for now.
There was a problem hiding this comment.
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.
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.
I'd prefer if we just keep it simple and use encodeURIComponent. Anything else feels opinionated to me.
lib/core/util.js
Outdated
| if (serializedParams) { | ||
| const hashmarkIndex = url.indexOf('#') | ||
| if (hashmarkIndex !== -1) { | ||
| url = url.slice(0, hashmarkIndex) |
There was a problem hiding this comment.
We can drop it, if you like
There was a problem hiding this comment.
sorry, change was done and test was added, but some garbage left behind
Co-authored-by: Robert Nagy <ronagy@icloud.com>
Co-authored-by: Robert Nagy <ronagy@icloud.com>
lib/core/util.js
Outdated
|
|
||
| for (const v of val) { | ||
| if (isDate(v)) { | ||
| throw new Error('Passing date as a query param is not supported, please serialize to string up-front') |
There was a problem hiding this comment.
Date is an object so the check below is sufficient.
|
Thanks |
|
Thank you so much! 🎉 |
fixes #1448