medic / cht-core

The CHT Core Framework makes it faster to build responsive, offline-first digital health apps that equip health workers to provide better care in their communities. It is a central resource of the Community Health Toolkit.
https://communityhealthtoolkit.org
GNU Affero General Public License v3.0
441 stars 211 forks source link

Replace deprecated "request-promise-native" dependency #6250

Open garethbowen opened 4 years ago

garethbowen commented 4 years ago

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.

dianabarsan commented 4 years ago

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 commented 4 years ago

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

latin-panda commented 4 years ago

Axios!

garethbowen commented 4 years ago

@latin-panda Why do you prefer axios?

mrsarm commented 4 years ago

+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:

dianabarsan commented 4 years ago

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.

latin-panda commented 4 years ago

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

dianabarsan commented 4 years ago

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 commented 2 years ago

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 commented 8 months ago

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.