node-vault icon indicating copy to clipboard operation
node-vault copied to clipboard

Replaced Request with Axios

Open pratikpc opened this issue 4 years ago • 12 comments

As is well known, the Request library has been deprecated.

As such, we need to replace Request with another.

A popular replacement is Axios

I have worked to ensure compatibility with Axios. For example, while Axios uses data param, Request uses json As such, I have retained both

One singular place this commit could be improved is in the portion dealing with Unit Tests

@kr1sp1n what's your view on this?

pratikpc avatar Mar 17 '20 21:03 pratikpc

Codecov Report

Merging #151 into master will not change coverage by %. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #151   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          139       156   +17     
  Branches        34        40    +6     
=========================================
+ Hits           139       156   +17     
Impacted Files Coverage Δ
src/index.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 17f06aa...2ca60d4. Read the comment docs.

codecov[bot] avatar Mar 17 '20 21:03 codecov[bot]

@pratikpc I am not saying we shouldnt replace request, but Axios is not the best choice IMHO. It's not updated regularly, it introduces breaking changes in minor updates etc. I myself am migrating away from Axios. There are other options out there.

Also request is a battle tested library. It does not need updates because it's just done. I agree with replacing deprecated software but not with worse software.

Kamahl19 avatar Mar 30 '20 17:03 Kamahl19

@Kamahl19 which library do you think can replace Request then if not Axios?

pratikpc avatar Mar 30 '20 22:03 pratikpc

What about got ?

fungiboletus avatar Apr 28 '20 07:04 fungiboletus

Any thoughts on this pull? I would love to utilize this package but dislike using a deprecated dependency for obvious reasons.

ultimumdesign avatar Oct 27 '20 17:10 ultimumdesign

Maybe consider https://github.com/nodejs/undici

tilgovi avatar Mar 05 '21 19:03 tilgovi

Can we please get this moving, no matter what library? This issue in/with jest is caused by request https://github.com/request/request-promise/issues/247

@pratikpc would you mind rebasing your PR to have it pass merge checks? @kr1sp1n what would it take to merge this? How can I help?

irnnr avatar Sep 24 '21 20:09 irnnr

any progress here? we're getting vulnerability issues because of the request

morielpahima avatar Oct 27 '21 10:10 morielpahima

Hi, Please fix and merge this PR we want to upgrade because of request has high-security vulnerabilities

rockrotem avatar Oct 27 '21 11:10 rockrotem

I would very much like to fix this but the moment any changes in node-vault take place, this would break again

So unless there is an interest in merging it, I don't really see much of a point to be honest.

pratikpc avatar Oct 27 '21 11:10 pratikpc

Axios is as battle-tested as they come. From their docs:

Until axios reaches a 1.0 release, breaking changes will be released with a new minor version. For example 0.5.1, and 0.5.4 will have the same API, but 0.6.0 will have breaking changes.

That means, barring human error (which can always happen), you can still depend on their versioning and stick to patch releases if you want to be on the safe side.

Can we get a maintainer to comment on willingness-to-merge if conflicts get resolved?

aphofstede avatar Nov 02 '21 12:11 aphofstede

@pratikpc Please resolve conflicts first. Thanks!

kr1sp1n avatar Nov 10 '21 07:11 kr1sp1n

this is conflicting and stale; Closing

[ nonetheless --> requests will be removed soon ]

aviadhahami avatar Nov 10 '22 17:11 aviadhahami

soon :) jk I'm happy to help out with the switch to axios/other alternatives

ChrisE217 avatar May 10 '23 12:05 ChrisE217

I'm really trying, tho I can't push to npm 🤷‍♂️

aviadhahami avatar May 10 '23 14:05 aviadhahami

@aviadhahami Hi, I created a npm org and will move the node-vault package to it so you can also manage the package. What is your npmjs handle?

kr1sp1n avatar May 11 '23 07:05 kr1sp1n

Ah, found it: uiw4nk3r Right? I invited you and when you are on board then I will move the package to this org.

kr1sp1n avatar May 11 '23 07:05 kr1sp1n

@kr1sp1n yes :) accepted it 👍 Note that I also require additional permissions to the repo in order to: create repository secrets (for deploy), remove stale CI/PR checks (travis #218), and probably future stuff 🙇‍♂️

aviadhahami avatar May 11 '23 08:05 aviadhahami

@aviadhahami You are admin of this repo now 🙌

kr1sp1n avatar May 11 '23 08:05 kr1sp1n

@aviadhahami Mmhh, still can't see you as a member of the new npmjs org "node-vault".

kr1sp1n avatar May 11 '23 08:05 kr1sp1n

@kr1sp1n not sure how to view that; I did accept your invite this morgen tho.... any ideas?

aviadhahami avatar May 11 '23 12:05 aviadhahami

@aviadhahami Did you activated 2FA on your npmjs account? I enforced 2FA on the org account. Maybe that's the problem?

kr1sp1n avatar May 11 '23 21:05 kr1sp1n

@kr1sp1n my 2FA is active 👍

aviadhahami avatar May 13 '23 21:05 aviadhahami