feat: migrate from deprecated request package to axios (twilio#81)

* feat: migrate from deprecated request library to axios * remove unnecessary header check
AsabuHere · Mar 13, 2020 · 2eba9ba · 2eba9ba
1 parent 2c8fb83
commit 2eba9ba
Show file tree

Hide file tree

Showing 7 changed files with 88 additions and 110 deletions.
diff --git a/.idea/workspace.xml b/.idea/workspace.xml
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -10,11 +10,12 @@
     "@oclif/errors": "^1.2.2",
     "@oclif/plugin-help": "^2.2.3",
     "@oclif/plugin-plugins": "^1.7.9",
+    "axios": "^0.19.2",
     "chalk": "^2.4.2",
     "columnify": "^1.5.4",
     "fs-extra": "^7.0.1",
     "inquirer": "^6.5.2",
-    "request": "^2.88.2",
+    "qs": "^6.9.1",
     "semver": "^6.3.0",
     "shelljs": "^0.8.3",
     "tsv": "^0.2.0",

diff --git a/src/services/cli-http-client.js b/src/services/cli-http-client.js
@@ -1,15 +1,18 @@
+var http_ = require('http');
+var https = require('https');
 const os = require('os');
 const pkg = require('../../package.json');
+const qs = require('qs');
 const { TwilioCliError } = require('../services/error');
 const { NETWORK_ERROR } = require('../services/messaging/help-messages');
 
-const NETWORK_ERROR_CODES = new Set(['ETIMEDOUT', 'ESOCKETTIMEDOUT']);
+const NETWORK_ERROR_CODES = new Set(['ETIMEDOUT', 'ESOCKETTIMEDOUT', 'ECONNABORTED']);
 
 class CliRequestClient {
   constructor(commandName, logger, http) {
     this.commandName = commandName;
     this.logger = logger;
-    this.http = require('util').promisify(http || require('request'));
+    this.http = http || require('axios');
   }
 
   /**
@@ -58,20 +61,26 @@ class CliRequestClient {
 
     const options = {
       timeout: opts.timeout || 30000,
-      followRedirect: opts.allowRedirects || false,
+      maxRedirects: opts.allowRedirects ? 10 : 0,
       url: opts.uri,
       method: opts.method,
       headers,
-      forever: opts.forever !== false
+      httpAgent: opts.forever ? new http_.Agent({ keepAlive: true }) : undefined,
+      httpsAgent: opts.forever ? new https.Agent({ keepAlive: true }) : undefined,
+      validateStatus: status => {
+        return status >= 100 && status < 600;
+      }
     };
 
     if (opts.data) {
-      options.formData = opts.data;
+      options.data = qs.stringify(opts.data, { arrayFormat: 'repeat' });
     }
 
     if (opts.params) {
-      options.qs = opts.params;
-      options.useQuerystring = true;
+      options.params = opts.params;
+      options.paramSerializer = params => {
+        return qs.stringify(params, { arrayFormat: 'repeat' });
+      };
     }
 
     this.lastRequest = options;
@@ -80,15 +89,19 @@ class CliRequestClient {
     try {
       const response = await this.http(options);
 
-      this.logger.debug('response.statusCode: ' + response.statusCode);
+      this.logger.debug('response.statusCode: ' + response.status);
       this.logger.debug('response.headers: ' + JSON.stringify(response.headers));
 
-      if (response.statusCode < 200 || response.statusCode >= 300) {
-        const parsed = JSON.parse(response.body);
+      if (response.status < 200 || response.status >= 300) {
+        const parsed = response.data;
         throw new TwilioCliError(`Error code ${parsed.code} from Twilio: ${parsed.message}. See ${parsed.more_info} for more info.`, parsed.code);
       }
 
-      return response;
+      return {
+        body: response.data,
+        statusCode: response.status,
+        headers: response.headers
+      };
     } catch (error) {
       if (NETWORK_ERROR_CODES.has(error.code)) {
         throw new TwilioCliError(NETWORK_ERROR);
@@ -102,14 +115,14 @@ class CliRequestClient {
     this.logger.debug('-- BEGIN Twilio API Request --');
     this.logger.debug(options.method + ' ' + options.url);
 
-    if (options.formData) {
+    if (options.data) {
       this.logger.debug('Form data:');
-      this.logger.debug(options.formData);
+      this.logger.debug(options.data);
     }
 
-    if (options.qs && Object.keys(options.qs).length > 0) {
+    if (options.params && Object.keys(options.params).length > 0) {
       this.logger.debug('Querystring:');
-      this.logger.debug(options.qs);
+      this.logger.debug(options.params);
     }
 
     this.logger.debug('User-Agent: ' + options.headers['User-Agent']);

diff --git a/src/services/open-api-client.js b/src/services/open-api-client.js
@@ -129,8 +129,7 @@ class OpenApiClient {
   }
 
   convertBody(responseBody, schema) {
-    const parsedBody = JSON.parse(responseBody);
-    return this.converter.convertSchema(schema, parsedBody);
+    return this.converter.convertSchema(schema, responseBody);
   }
 
   evaluateRefs(schema, domain) {

diff --git a/test/services/cli-http-client.test.js b/test/services/cli-http-client.test.js
@@ -10,9 +10,9 @@ describe('services', () => {
     });
 
     test.it('should make an http request', async () => {
-      const client = new CliRequestClient('blah', logger, (options, callback) => {
+      const client = new CliRequestClient('blah', logger, options => {
         expect(options.url).to.equal('https://foo.com/bar');
-        callback(null, { statusCode: 200, body: '{}' });
+        return { status: 200, data: 'foo', headers: {} };
       });
       expect(client.commandName).to.equal('blah');
       const response = await client.request({
@@ -26,7 +26,7 @@ describe('services', () => {
       });
 
       expect(response.statusCode).to.equal(200);
-      expect(response.body).to.equal('{}');
+      expect(response.body).to.equal('foo');
     });
 
     test

diff --git a/test/services/twilio-api/twilio-client.test.js b/test/services/twilio-api/twilio-client.test.js
@@ -2,6 +2,7 @@ const { expect, test, constants } = require('@twilio/cli-test');
 const { logger } = require('../../../src/services/messaging/logging');
 const { TwilioApiClient } = require('../../../src/services/twilio-api');
 const CliRequestClient = require('../../../src/services/cli-http-client');
+const qs = require('qs');
 
 const accountSid = constants.FAKE_ACCOUNT_SID;
 const callSid = 'CA12345678901234567890123456789012';
@@ -114,7 +115,7 @@ describe('services', () => {
           });
 
           expect(response).to.eql({ status: 'ringing' });
-          expect(httpClient.lastRequest.formData).to.eql({ To: '123', From: '456' });
+          expect(httpClient.lastRequest.data).to.eql(qs.stringify({ To: '123', From: '456' }));
         });
 
       test
@@ -148,7 +149,7 @@ describe('services', () => {
           });
 
           expect(response).to.eql({ status: 'canceled' });
-          expect(httpClient.lastRequest.formData).to.eql({ Status: 'canceled' });
+          expect(httpClient.lastRequest.data).to.eql(qs.stringify({ Status: 'canceled' }));
         });
 
       test
@@ -228,7 +229,7 @@ describe('services', () => {
           });
 
           expect(response).to.eql({ verified: 'true' });
-          expect(httpClient.lastRequest.formData).to.eql({ EmergencyEnabled: 'true' });
+          expect(httpClient.lastRequest.data).to.eql(qs.stringify({ EmergencyEnabled: 'true' }));
         });
 
       /* eslint-disable max-nested-callbacks */