node-vault
node-vault copied to clipboard
Replaced Request with Axios
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?
Codecov Report
Merging #151 into master will not change coverage by
%
. The diff coverage is100.00%
.
@@ 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.
@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 which library do you think can replace Request then if not Axios?
What about got
?
Any thoughts on this pull? I would love to utilize this package but dislike using a deprecated dependency for obvious reasons.
Maybe consider https://github.com/nodejs/undici
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?
any progress here? we're getting vulnerability issues because of the request
Hi, Please fix and merge this PR we want to upgrade because of request has high-security vulnerabilities
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.
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?
@pratikpc Please resolve conflicts first. Thanks!
this is conflicting and stale; Closing
[ nonetheless --> requests
will be removed soon ]
soon :) jk I'm happy to help out with the switch to axios/other alternatives
I'm really trying, tho I can't push to npm 🤷♂️
@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?
Ah, found it: uiw4nk3r Right? I invited you and when you are on board then I will move the package to this org.
@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 You are admin of this repo now 🙌
@aviadhahami Mmhh, still can't see you as a member of the new npmjs org "node-vault".
@kr1sp1n not sure how to view that; I did accept your invite this morgen tho.... any ideas?
@aviadhahami Did you activated 2FA on your npmjs account? I enforced 2FA on the org account. Maybe that's the problem?
@kr1sp1n my 2FA is active 👍