chore: switch to axios #182

JustinBeckwith · 2017-11-18T03:59:39Z

Switching out request for axios, and making the API support callback and async styles.

JustinBeckwith · 2017-11-19T07:41:11Z

@ofrobots all the tests are passing, this is ready for a pass. Apologies for the size of the review, couldn't really find a way to split it up.

package.json

  },
  "devDependencies": {
+    "@types/jws": "^3.1.0",


src/auth/authclient.ts

@@ -28,5 +34,5 @@ export abstract class AuthClient {
   * Provides an alternative request
   * implementations with auth credentials.
   */
-  abstract request(opts: request.Options): request.Request|void;
+  abstract request(opts: AxiosRequestConfig): Promise<BodyResponse>;


src/transporters.ts

-  request(opts: request.Options, callback?: BodyResponseCallback) {
+  request(opts: AxiosRequestConfig): AxiosPromise;
+  request(opts: AxiosRequestConfig, callback?: BodyResponseCallback): void;
+  request(opts: AxiosRequestConfig, callback?: BodyResponseCallback):


src/auth/authclient.ts


 import {DefaultTransporter} from '../transporters';

 import {Credentials} from './credentials';

+export interface BodyResponse {
+  // tslint:disable-next-line no-any
+  body: any;


src/transporters.ts

-        body = JSON.parse(body as string);
-      } catch (err) {
-        /* no op */
+  private processError(e: AxiosError): RequestError {


JustinBeckwith · 2017-11-22T21:06:24Z

@ofrobots I think I covered all the feedback so far. Ready for the next wave :)

ofrobots

Some initial comments. Still want going through the rest of the file, but ran out of time.

src/auth/computeclient.ts

+    try {
+      const url = this.opts.tokenUrl || Compute._GOOGLE_OAUTH2_TOKEN_URL;
+      // request for new token
+      const res = await this.transporter.request({url});


src/auth/computeclient.ts

-        });
+  protected async refreshToken(refreshToken?: string|
+                               null): Promise<GetTokenResponse> {
+    try {


src/auth/credentials.ts

@@ -19,6 +19,7 @@ export interface Credentials {
  expiry_date?: number;
  access_token?: string;
  token_type?: string;
+  expires_in?: number;


src/auth/googleauth.ts

-  getDefaultProjectId(callback: ProjectIdCallback) {
+  getDefaultProjectId(): Promise<string>;
+  getDefaultProjectId(callback: ProjectIdCallback): void;
+  getDefaultProjectId(callback?: ProjectIdCallback): Promise<string|null>|void {


src/auth/googleauth.ts

-          return;
-        }
+    // environment variable
+    projectId = this.getProductionProjectId();


src/auth/googleauth.ts

-      callback:
-          (error: Error|null, stdout: string, stderr: string|null) => void) {
-    exec('gcloud -q config list core/project --format=json', callback);
+  _getSDKDefaultProjectId():


ofrobots

still more to come

src/auth/googleauth.ts

+      if (gce) {
+        // For GCE, just return a default ComputeClient. It will take care of
+        // the rest.
+        return {projectId: null, credential: new Compute()};


src/auth/googleauth.ts

    let client: UserRefreshClient|JWT;
-    if (!json) {
+    try {
+      if (!json) {


src/auth/googleauth.ts

-      json: JWTInput,
-      callback?: (err: Error|null, client?: UserRefreshClient|JWT) => void) {
+  fromJSON(json: JWTInput): JWT|UserRefreshClient;
+  fromJSON(json: JWTInput, callback: CredentialCallback): void;


src/auth/googleauth.ts

@@ -453,15 +486,23 @@ export class GoogleAuth {
   * @param {function=} - Optional callback function
   */
  fromAPIKey(
-      apiKey: string, callback?: (err?: Error|null, client?: JWT) => void) {
+      apiKey: string,


src/auth/jwtaccess.ts

-          (err: Error|null, headers?: http.IncomingHttpHeaders|null) => void) {
-    const iat = Math.floor(new Date().getTime() / 1000);
-    const exp = iat + 3600;  // 3600 seconds = 1 hour
+  getRequestMetadata(authURI: string): RequestMetadataResponse;


src/auth/jwtclient.ts

-      } else {
-        return newGToken.getToken((err2?: Error|null, token?: string|null) => {
-          return done(err2, {
+  async refreshToken(refreshToken?: string|null) {


src/auth/jwtclient.ts

-          });
-        });
-      }
+          } as Credentials;


src/auth/jwtclient.ts

-      done(new Error(
-          'The incoming JSON object does not contain a private_key field'));
-      return;
+  fromJSON(json: JWTInput, callback?: (err?: Error) => void): void {


src/auth/jwtclient.ts

-        done(err);
+  }
+
+  private async fromStreamAsync(inputStream: stream.Readable) {


src/auth/jwtclient.ts

@@ -215,37 +233,36 @@ export class JWT extends OAuth2Client {
   * @param {function=} callback - Optional callback to be invoked after
   *  initialization.
   */
-  fromAPIKey(apiKey: string, callback?: (err: Error) => void) {
-    const done = callback || noop;
+  fromAPIKey(apiKey: string, callback?: (err?: Error) => void): void {


src/auth/oauth2client.ts

+    const res = await this.transporter.request<CredentialRequest>(
+        {method: 'POST', url, data});
+    const tokens = res.data as Credentials;
+    if (res.data && res.data.expires_in) {


src/auth/oauth2client.ts

    } else {
-      return callback(null, this.credentials.access_token, null);
+      return {token: this.credentials.access_token, res: undefined};


src/auth/oauth2client.ts

+    const headers = {
+      Authorization: credentials.token_type + ' ' + tokens.access_token
+    };
+    return {headers, res: r ? r.res : null};


src/auth/refreshclient.ts

-      if (callback) {
-        callback(new Error(
-            'Must pass in a JSON object containing the user refresh token'));
+    try {


src/options.ts

@@ -0,0 +1,33 @@
+import {AxiosRequestConfig} from 'axios';


test/test.jwt.ts

@@ -643,7 +612,7 @@ describe('.fromAPIKey', () => {
    it('should set the .apiKey property on the instance', (done) => {
      jwt.fromAPIKey(KEY, (err) => {
        assert.strictEqual(jwt.apiKey, KEY);
-        assert.strictEqual(err, null);
+        assert.equal(err, null);


test/test.oauth2.ts

@@ -855,7 +854,7 @@ describe('OAuth2 client', () => {
    const auth = new GoogleAuth();
    const oauth2client =
        new auth.OAuth2(CLIENT_ID, CLIENT_SECRET, REDIRECT_URI);
-    oauth2client.request(({} as request.OptionsWithUri), (err, result) => {
+    oauth2client.request(({} as AxiosRequestConfig), (err, result) => {


test/test.oauth2.ts

@@ -996,7 +995,10 @@ describe('OAuth2 client', () => {
      oauth2client.credentials = {access_token: 'abc', refresh_token: 'abc'};
      oauth2client.revokeCredentials((err, result) => {
        assert.equal(err, null);
-        assert.equal(result.success, true);
+        assert(result);


test/test.oauth2.ts

-        assert(tokens.expiry_date >= now + (10 * 1000));
-        assert(tokens.expiry_date <= now + (15 * 1000));
+        assert(tokens);
+        if (tokens && tokens.expiry_date) {


test/test.transporters.ts

  const defaultUserAgentRE = 'google-api-nodejs-client/\\d+.\\d+.\\d+';
  const transporter = new DefaultTransporter();

  it('should set default client user agent if none is set', () => {
-    const opts = transporter.configure(({} as request.OptionsWithUrl));
+    const opts = transporter.configure(({} as AxiosRequestConfig));


JustinBeckwith requested a review from ofrobots November 18, 2017 03:59

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 18, 2017

JustinBeckwith mentioned this pull request Nov 18, 2017

Replace request with http. #166

Closed

JustinBeckwith added this to the 1.0 milestone Nov 19, 2017

ofrobots reviewed Nov 19, 2017

View reviewed changes

package.json Outdated

},

"devDependencies": {

"@types/jws": "^3.1.0",

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

ofrobots reviewed Nov 19, 2017

View reviewed changes

JustinBeckwith changed the base branch from master to next November 21, 2017 02:09

ofrobots reviewed Nov 21, 2017

View reviewed changes

src/transporters.ts Outdated

body = JSON.parse(body as string);

} catch (err) {

/* no op */

private processError(e: AxiosError): RequestError {

This comment was marked as spam.

Sign in to view

JustinBeckwith added 3 commits November 21, 2017 10:40

chore: switch to axios

73a4855

fix: broken getCredential tests

09889b3

Make request calls generic and remove body responses

8f84652

ensure axios config and update jsdoc

72a8677

ofrobots reviewed Nov 22, 2017

View reviewed changes

JustinBeckwith added 2 commits November 22, 2017 14:55

code review feedback

f71e7b7

remove makeRequest

6a93f0d

ofrobots reviewed Nov 23, 2017

View reviewed changes

JustinBeckwith mentioned this pull request Nov 23, 2017

Remove unnecessary callbacks from sync APIs #188

Closed

7 tasks

JustinBeckwith added 2 commits November 22, 2017 18:55

more PR feedback

bf8f693

attempt to fix test failures

c1ef868

ofrobots reviewed Nov 24, 2017

View reviewed changes

PR feedback

6b4e4cf

ofrobots approved these changes Nov 24, 2017

View reviewed changes

JustinBeckwith merged commit 98b2b1a into googleapis:next Nov 24, 2017

JustinBeckwith mentioned this pull request Nov 26, 2017

Request callback function arguments are not in correct order #102

Closed

ofrobots pushed a commit to ofrobots/google-auth-library-nodejs that referenced this pull request Dec 1, 2017

chore: switch to axios (googleapis#182)

c7c2798

ofrobots pushed a commit that referenced this pull request Jan 9, 2018

chore: switch to axios (#182)

7be6f77

tnguyen14 mentioned this pull request Mar 1, 2018

Implement backwards compatible promise support. googleapis/google-api-nodejs-client#559

Closed

chore: switch to axios #182

chore: switch to axios #182

Conversation

JustinBeckwith commented Nov 18, 2017 • edited

JustinBeckwith commented Nov 19, 2017

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

JustinBeckwith commented Nov 22, 2017

ofrobots left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

ofrobots left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

JustinBeckwith commented Nov 18, 2017 •

edited