Skip to content
This repository was archived by the owner on Aug 7, 2021. It is now read-only.

fix(snapshot): use request module for http requests #428

Merged
merged 5 commits into from
Feb 15, 2018
Merged

fix(snapshot): use request module for http requests #428

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4d092ed
fix(snapshot): use request module for http request
sis0k0 Jan 29, 2018
011ce3c
chore: pin dependencies versions
sis0k0 Feb 13, 2018
9b8b922
fix(snapshot): reject with full response when request in not successful
sis0k0 Feb 13, 2018
0733d8e
refactor: use proxy-lib to get request options
sis0k0 Feb 13, 2018
b6239e2
fix: return on reject
sis0k0 Feb 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
"dependencies": {
"minimatch": "^3.0.4",
"nativescript-hook": "0.2.2",
"request": "^2.83.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should use strict versions in package.json. This minimizes the risk of being broken by release of another package.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll pin the versions.

"schema-utils": "^0.4.3",
"semver": "^5.4.1",
"shelljs": "^0.6.0"
Expand Down
62 changes: 24 additions & 38 deletions snapshot/android/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
const { chmodSync, createWriteStream, existsSync } = require("fs");
const { get: httpsGet } = require("https");
const { tmpdir } = require("os");
const { dirname, join } = require("path");

const { mkdir } = require("shelljs");
const { get } = require("request");

const CONSTANTS = {
SNAPSHOT_TMP_DIR: join(tmpdir(), "snapshot-tools"),
Expand All @@ -12,49 +13,34 @@ const createDirectory = dir => mkdir('-p', dir);

const downloadFile = (url, destinationFilePath) =>
new Promise((resolve, reject) => {
const request = httpsGet(url, response => {
switch (response.statusCode) {
case 200:
const file = createWriteStream(destinationFilePath, {autoClose: true});
file.on('error', function (error) {
return reject(error);
});
file.on("finish", function() {
chmodSync(destinationFilePath, 0755);
return resolve(destinationFilePath);
});
response.pipe(file);
break;
case 301:
case 302:
case 303:
const redirectUrl = response.headers.location;
return this.downloadExecFile(redirectUrl, destinationFilePath);
default:
return reject(new Error("Unable to download file at " + url + ". Status code: " + response.statusCode));
}
});

request.end();

request.on('error', function(err) {
return reject(err);
});
get(url)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any tests for this functionality are preferable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

.on("error", reject)
.pipe(createWriteStream(destinationFilePath, { autoClose: true }))
.on("finish", _ => {
chmodSync(destinationFilePath, 0755);
resolve(destinationFilePath);
});
});

const getJsonFile = url =>
new Promise((resolve, reject) => {
httpsGet(url, res => {
let body = "";
res.on("data", chunk => {
body += chunk;
})
get(url, (error, response, body) => {
if (error) {
reject(error);
Copy link
Contributor

@Plamen5kov Plamen5kov Feb 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return reject(error)

}

const { statusCode } = response;
if (statusCode !== 200) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

reject(`Couldn't fetch ${url}! Response status code: ${statusCode}`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Plamen5kov Plamen5kov Feb 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return reject(...) again to prevent unexpected behavior with json parse

}

res.on("end", () => {
try {
const data = JSON.parse(body);
return resolve(data);
});
}).on("error", reject);
resolve(data);
} catch (e) {
reject(`Couldn't parse json data! Original error:\n${e}`);
}
});
});

module.exports = {
Expand Down