feat(#6250): use fetch in couch-request #9746
Conversation
Signed-off-by: Diana Barsan <[email protected]>
Signed-off-by: Diana Barsan <[email protected]>
Signed-off-by: Diana Barsan <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Diana Barsan <[email protected]>
- handing server sending JSON headers but no content - double slash paths - from data -> json Signed-off-by: Diana Barsan <[email protected]>
Signed-off-by: Diana Barsan <[email protected]>
Signed-off-by: Diana Barsan <[email protected]>
Signed-off-by: Diana Barsan <[email protected]>
Signed-off-by: Diana Barsan <[email protected]>
Signed-off-by: Diana Barsan <[email protected]>
Signed-off-by: Diana Barsan <[email protected]>
Signed-off-by: Diana Barsan <[email protected]>
|
Hi @jkuester ! Would you mind with some feedback over here? Appreciate it!!! |
|
👍 Will have a look asap, but might not be until Friday. |
There was a problem hiding this comment.
Send it! Just a few minor non-blocking comments.
(Also just have to say that I hate JS. With no type safety this kind of refactor is terrifying. AFICT you caught all the user > username, pass > password, and statusCode > status changes, but it is really hard to tell....)
|
|
||
| const req = (method, first, second = {}) => { | ||
| if (options.auth) { |
There was a problem hiding this comment.
question: I wonder if we should throw an exception if we have both options.auth and options.headers.authorization set? (Out of an abundance of caution.) Just very much don't want there to be a misunderstanding about which auth creds are being used for a request.
There was a problem hiding this comment.
I thought about this as well. Now that there are two of us that think about it, I firmly believe it's a good idea :)
| setRequestUri(options); | ||
| setRequestAuth(options); | ||
| setTimeout(options); | ||
| const sendJson = setRequestContentType(options); |
There was a problem hiding this comment.
nitpick: it would read better if you called getSendJson up here and then passed in sendJson as a parameter to setRequestContentType (just weird to return a value from set...).
| case methods.HEAD: { | ||
| return request.head; | ||
|
|
||
| return { options, sendJson }; |
There was a problem hiding this comment.
suggestion: actually, what if we just did not return anything from this function at all? As things are, it is pretty strange that the function accepts the options parameter, mutates the options parameter, then returns an object containing the mutated options parameter. This is not at all how I expected this to work when I first glanced at the code.
Instead, we could change the function name to something like mutateRequestOptions and return nothing. The request function below should be able to calculate const sendJson = options.headers[CONTENT_TYPE] === JSON_HEADER_VALUE; right?
| if (!authHeader || !authHeader.startsWith('Basic ')) { | ||
| return false; | ||
| } | ||
| try { | ||
| const [username, password] = Buffer.from(authHeader.split(' ')[1], 'base64').toString().split(':'); | ||
| const [username, password] = atob(authHeader.split(' ')[1]).split(':'); |
There was a problem hiding this comment.
nitpick: why change this? Looks like atob is marked as "legacy" in favor of Buffer.from...
There was a problem hiding this comment.
Oh, my bad. I'll switch it back!
Description
Removes
request-promise-native.#6250
Code review checklist
can_view_old_navigationpermission to see the old design.Compose URLs
If Build CI hasn't passed, these may 404:
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.