-
Notifications
You must be signed in to change notification settings - Fork 656
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 npm audit
complaining that elm is still using request
#2306
base: master
Are you sure you want to change the base?
Conversation
Thanks for suggesting these code changes. To set expectations:
Finally, please be patient with the core team. They are trying their best with limited resources. |
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.
It’s good that https://github.com/axios/axios mentions a http_proxy
environment variable! Because that was one of the reasons for using request
instead of plain Node.js in the first place.
However, for this to be merged I guess testing / “proof” that there are no proxy stuff regressions compared to the request
version would be needed.
@@ -53,7 +53,7 @@ module.exports = function(callback) | |||
}); | |||
|
|||
// put it all together | |||
request(url).on('error', reportDownloadFailure).pipe(gunzip).pipe(write); | |||
request.get(url).then(gunzip).then(write).catch(reportDownloadFailure); |
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.
When I run node install.js
on master, it replaces bin/elm
(a Node.js script) with an elm binary, which is the expected behavior.
On your branch, node install.js
results in bin/elm
becoming an empty file.
.pipe
is for chaining streams, while .then
is for chaining promises?
I tried implementing it using https from the Node.js standard library, but the elm download URL needs to follow a redirect, and it seems to work better if implemented with the follow-redirect library instead. My experimental implementation is here: https://github.com/mather/elm-compiler/blob/82398ec1ced4dd9afd3380521f16b44e54a294c9/installers/npm/download.js |
Quick Summary:
request has been deprecated, and
npm audit
complains about it. This PR simply removes request and replaces it with axios, semantics are equivalent.Additional Info:
I understand that we have the #2287 PR to overhaul the current npm installation, but it seems that progress there has been extremely slow.