-
Notifications
You must be signed in to change notification settings - Fork 163
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
fix: move from request module to axios #266
Conversation
a545bdc
to
9c02b5d
Compare
9c02b5d
to
692b0a7
Compare
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@@ -12,6 +12,7 @@ | |||
"lodash": "^4.17.11", | |||
"node-rsa": "^1.0.5", | |||
"request": "^2.72.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also remove the dependency.
@@ -12,6 +12,7 @@ | |||
"lodash": "^4.17.11", | |||
"node-rsa": "^1.0.5", | |||
"request": "^2.72.0", | |||
"axios": "^0.19.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I have PTSD from this library.
Look no further than: https://github.com/axios/axios/blob/master/lib/core/mergeConfig.js#L13-L72
I don't know what to recommend instead, but I would strongly recommend to avoid axios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node's own http library is pretty trivial to get working, I use it whenever possible to connect to API endpoints
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the |
@avindra why was Axios avoided, it could have replaced the request. Axios is actively being maintained and there were no active security issues now. |
@vv619-perf axios still has:
I might do a more extended writeup on this, because it seems that half the JS people out there are haplessly using axios without knowing the real issues. |
Is there a preferred library to use to replace request? I am interested in having this resolved, but am tentative to select a library without getting approval first. Would superagent be suitable? |
fixes: #262
Checklist
guide