cht-core icon indicating copy to clipboard operation
cht-core copied to clipboard

Replace deprecated "request-promise-native" dependency

Open garethbowen opened this issue 5 years ago • 10 comments

Describe the issue The request package (which is required by request-promise-native) has been deprecated so we need to find an alternative which is being maintained.

Describe the improvement you'd like Pick another library that is being maintained. Some options are listed here: https://github.com/request/request/issues/3143

Describe alternatives you've considered We could stay with request-promise-native for a while and only switch when an issue is found but I'd prefer to get a head start on this.

garethbowen avatar Feb 18 '20 20:02 garethbowen

This is a probably not totally accurate side-by-side api comparison:

Request Axios Node fetch
Url || uri url url
baseUrl baseUrl -
method method method
headers: Object headers: Object headers: Headers
qs: Object params: Object -
qsParseOptions - -
qsStringifyOptions paramsSerializer -
useQuerystring - -
body: Object|any data Object|any body: null|string|buffer|stream|blob
form data body
formData -
multipart -
preambleCRLF partially transformRequest
postambleCRLF partially transformRequest
json partially responseType .json
jsonReviver partially transformRequest
jsonReplacer partially transformRequest
auth auth
oauth -
hawk -
aws -
httpSignature -
followRedirect maxRedirects = 0 redirect
followAllRedirects -
followOriginalHttpMethod -
maxRedirects maxRedirects follow
removeRefererHeader -
encoding responseEncoding
gzip decompress compress
jar -
agent httpAgent agent
agentClass -
agentOptions -
forever -
pool -
timeout timeout somewhat possible using abort-controller: https://github.com/node-fetch/node-fetch#request-cancellation-with-abortsignal
localAddress -
proxy proxy
strictSSL -
tunnel -
proxyHeaderWhiteList -
proxyHeaderExclusiveList -
time -
har -
callback -

dianabarsan avatar Aug 24 '20 16:08 dianabarsan

I still like request-promise-native the best, I think it offers the richest api.

In terms of ease of use, I think axios comes first, node-fetch wins size and dependecies category.

In terms of npm/git stats:

Thing request + rpn axios node-fetch
Install size* 5.7MiB (1668 files)** 422KiB (48 files) 163KiB (8 files)
dependencies 20 + 3 1 0
Weekly downloads 19kk + 9kk 10kk 16kk
Open issues 135 + 12 212 56
Open bugs no issue labels 41 7
Open PRs 46 + 2 59 11
Commits 2270 + 55 961 534
Contributors 312 + 6 252 72
"Used by" 5.6kk + 1.9kk 2.9kk 2kk
Releases this year 0 + 1 5 0
Contributions 1 month (people) 0 3(2) 7(4)
Vulnerabilities (year, type)*** 1 + 0 (2016 - Remote memory exposure) 1 (2019 DoS) 0

*Running npm install <dependency> on a project with no dependencies and checking the disk space taken up by node_modules. **Installing request and request-promise-native. ***Checking vulnerabilities on https://snyk.io

dianabarsan avatar Aug 25 '20 03:08 dianabarsan

Axios!

latin-panda avatar Aug 25 '20 18:08 latin-panda

@latin-panda Why do you prefer axios?

garethbowen avatar Aug 25 '20 22:08 garethbowen

+1 Axios. I used it in the past and it is simple, and works in both the browser and the server (despite we are planning for now just a replacement for scripts and backends). Furthermore:

  • Axios handles non 2xx HTTP responses as errors (returning a rejected promise, and therefore you are able to use try {...} catch {..} in a async function), while node-fetch no, and there is no way to create a "node-fetch" object with automatic handling of errors in certain way, you need to chain the error handler each call: https://www.npmjs.com/package/node-fetch#handling-client-and-server-errors
  • Axios, like request, allows to create a client object where you can define boilerplate configurations for each request, like timeout, baseUrl, headers... with node-fetch you need to merge your base configuration with the specifics for the request each time.

mrsarm avatar Aug 26 '20 13:08 mrsarm

Agree that Axios might be preferable in terms of migration effort, since it has an API similar enough to request-promise-native and has similar error handling capabilities.

dianabarsan avatar Aug 26 '20 13:08 dianabarsan

Hi @garethbowen ! To complement the previous responses: It also has some nice features like canceling requests, supports interceptors and it's TypeScript friendly (if we decide to type in the future). It's just very simple to use and it's weight is okay, not too heavy as other libs. It's promise based so it's nothing "new" which makes it feel more "standard" to the language. It works in any project JS based (BE & FE) 🙂

latin-panda avatar Aug 26 '20 22:08 latin-panda

I think both have merit.

In regards to what feels "Standard" (re @latin-panda), I would say that fetch is the standard, by being the main API provided by browsers. Not that the standard is necessarily good or intuitive to use (who among us can say they actaually liked XMLHttpRequest and didn't jump head first into using jQuery.ajax() when it was available?).

Have I ever reached to npm install node-fetch when writing a script? No. Have I ever reached to npm install axios when writing a script? Multiple times.

dianabarsan avatar Aug 27 '20 06:08 dianabarsan

I have one argument against node-fetch: it's annoying to stub in unit tests because of the default export.

This is how others are trying to do it.. All options looks pretty bad.

dianabarsan avatar Feb 03 '22 16:02 dianabarsan

It seems there are some unwanted interactions between Node 20 and request-promise-native: https://github.com/medic/cht-core/issues/8868#issuecomment-1959546903

I've removed the low priority tag, as this is blocking us from confidently upgrading NodeJs.

dianabarsan avatar Feb 22 '24 14:02 dianabarsan