-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
update request to 2.87.0 #1471
update request to 2.87.0 #1471
Conversation
updated request to include security fixes and make it same as npm current Passes all tests.
Hello @Rohithzr and thank you for the contribution! The only issue this might have is that it's incompatible with node <= 4. We planned on releasing a Lines 37 to 39 in 50b8734
Let's see if our CI is properly setup - https://ci.nodejs.org/job/nodegyp-test-pull-request/54/ |
I guess I was wrong and |
Ah! I actually tested it with node 4.x which is still used somewhere in our legacy code. didn't give me any issues. Thanks for confirming it. |
package.json
Outdated
@@ -28,7 +28,7 @@ | |||
"nopt": "2 || 3", | |||
"npmlog": "0 || 1 || 2 || 3 || 4", | |||
"osenv": "0", | |||
"request": "2", | |||
"request": "^2.86.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.
Although the version should match, maybe use 2.87.0 since they removed the heok/hawk issues https://github.com/request/request/blob/master/CHANGELOG.md#v2870-20180521
@refack I guess this PR can be merged now. |
FWIW we did this update in node-sass@4.9.1 to resolve the security warnings and maintains support for Node 0.10. It was a smooth update. |
@Fishrock123 so it works here without a doubt. How about we merge this and also figure out the 3.x. |
updated request to include security fixes and make it same as npm current
Passes all tests.