Skip to content
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

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

Closed
garethbowen opened this issue Feb 18, 2020 · 12 comments
Closed

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

garethbowen opened this issue Feb 18, 2020 · 12 comments
Assignees
Labels
Dependencies Update the project's dependencies/libraries Type: Technical issue Improve something that users won't notice
Milestone

Comments

@garethbowen
Copy link
Contributor

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: request/request#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 garethbowen added Type: Technical issue Improve something that users won't notice Priority: 3 - Low Can be bumped from the release labels Feb 18, 2020
@garethbowen garethbowen added the Help wanted Good for first time contributions label Mar 31, 2020
@dianabarsan
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

Axios!

@garethbowen
Copy link
Contributor Author

@latin-panda Why do you prefer axios?

@mrsarm
Copy link
Contributor

mrsarm commented Aug 26, 2020

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

@dianabarsan
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member

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.

@latin-panda latin-panda added the Dependencies Update the project's dependencies/libraries label Dec 20, 2022
@dianabarsan dianabarsan removed the Priority: 3 - Low Can be bumped from the release label Feb 22, 2024
@dianabarsan
Copy link
Member

It seems there are some unwanted interactions between Node 20 and request-promise-native: #8868 (comment)

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

@dianabarsan
Copy link
Member

@garethbowen says that since we've now upgraded to Node 22, we can just use the built-in fetch instead.
This became an easy change now.

@dianabarsan dianabarsan self-assigned this Dec 5, 2024
dianabarsan added a commit that referenced this issue Dec 13, 2024
Updates e2e tests request to use built-in fetch instead of request-promise-native.
Removes sessions from e2e tests, these are not needed anymore.
Updates all (except 2) datasource e2e tests to query api directly. These tests required that global fetch had a session enabled to work, and stopped working once sessions were removed.

#6250
dianabarsan added a commit that referenced this issue Jan 10, 2025
Switches to fetch is support or build scripts.
Removes deprecated generate-form-attachments script. As of 4.x, reports no longer have xml attachments.
#6250
@dianabarsan dianabarsan added this to the 4.17.0 milestone Jan 21, 2025
@dianabarsan dianabarsan removed the Help wanted Good for first time contributions label Jan 21, 2025
@dianabarsan
Copy link
Member

Should I have waited until Feb 18th? To have a 5 year anniversary on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dependencies Update the project's dependencies/libraries Type: Technical issue Improve something that users won't notice
Projects
None yet
Development

No branches or pull requests

4 participants