fix(snapshot): use request module for http requests #428
Conversation
Use 'request' node package, which respects environment proxy variables (i.e. HTTPS_PROXY). fixes #389
There was a problem hiding this comment.
This PR is a step in the right direction, but I see one problem at the moment. CLI uses proxy-lib to get the settings for proxy and passes them to request module. So CLI does not support HTTPS_PROXY variable (as proxy-lib does not support it yet). So in current situation, users who have proxy will have to set CLI proxy (via commands) and HTTPS_PROXY variable(s).
Maybe we should add proxy-lib as dependency here and pass the settings it returns to request module. This way, when CLI proxy is set, everything will work. Also, once we add support for HTTPS_PROXY in proxy-lib, it will be propagated to all packages that depend on this library.
package.json
Outdated
| @@ -42,6 +42,7 @@ | |||
| "dependencies": { | |||
| "minimatch": "^3.0.4", | |||
| "nativescript-hook": "0.2.2", | |||
| "request": "^2.83.0", | |||
There was a problem hiding this comment.
IMO we should use strict versions in package.json. This minimizes the risk of being broken by release of another package.
There was a problem hiding this comment.
I'll pin the versions.
snapshot/android/utils.js
Outdated
| } | ||
|
|
||
| const { statusCode } = response; | ||
| if (statusCode !== 200) { |
There was a problem hiding this comment.
Maybe we should accept all 2xx codes as success. Also you may receive 3xx (redirect). You may handle this by passing followAllRedirects option to request module - this way you'll not receive 3xx here, as request module will handle them for you.
There was a problem hiding this comment.
2xx - Correct me if I'm wrong but I'm sure that all the 2xx codes will denote success in this case. For example, status code 204 ('No content') doesn't seem to be ok when we are expecting to fetch a file or receive a json.
3xx - According to the docs, following GET HTTP responses is turned on by default. I'm not sure if we should follow non-GET HTTP responses (followAllRedirects: true), but I may be missing something. Is there a reason to turn it on?
snapshot/android/utils.js
Outdated
|
|
||
| const { statusCode } = response; | ||
| if (statusCode !== 200) { | ||
| reject(`Couldn't fetch ${url}! Response status code: ${statusCode}`) |
There was a problem hiding this comment.
In this case it is a good idea to show the real response. For example when you have proxy that has blocked access to this resource, the response may contain information that the admins of the user have blocked the access and who is responsible for enabling the access.
There was a problem hiding this comment.
return reject(...) again to prevent unexpected behavior with json parse
snapshot/android/utils.js
Outdated
| }) | ||
| get(url, (error, response, body) => { | ||
| if (error) { | ||
| reject(error); |
snapshot/android/utils.js
Outdated
| request.on('error', function(err) { | ||
| return reject(err); | ||
| }); | ||
| get(url) |
There was a problem hiding this comment.
any tests for this functionality are preferable
There was a problem hiding this comment.
I left it without tests, because I thought that the tests for the get method inside the request package are enough. I agree that it would be good to test our specific functionality. Can you suggest some scenarios that I can add?
| let body = ""; | ||
| res.on("data", chunk => { | ||
| body += chunk; | ||
| const options = { url }; |
There was a problem hiding this comment.
what about followAllRedirects option? Do you think we should add it by default?
Use 'request' node package for https requests and 'proxy-lib' for getting configured proxy settings (see: https://github.com/NativeScript/nativescript-cli/blob/master/docs/man_pages/general/proxy-set.md).
fixes #389