google-auth-library-nodejs icon indicating copy to clipboard operation
google-auth-library-nodejs copied to clipboard

Refactoring: Quit mixing Promises and async/await

Open shogo-nakano-desu opened this issue 2 years ago • 2 comments

Problem

  • There is a comment by @bcoe that says TODO. This comment has been here for two years.
    • https://github.com/googleapis/google-auth-library-nodejs/blob/main/src/auth/googleauth.ts#L206-L232

Suggestion

  • It would be better to use async/await only and reduce the length of code. It will make this repo easier to see through.
  • I am going to create PR when this change is accepted but beforehand, I put my plan below. Please use these code for your decision.

Before

// TODO: refactor the below code so that it doesn't mix and match
      // promises and async/await.
      this._getDefaultProjectIdPromise = new Promise(
        // eslint-disable-next-line no-async-promise-executor
        async (resolve, reject) => {
          try {
            const projectId =
              this.getProductionProjectId() ||
              (await this.getFileProjectId()) ||
              (await this.getDefaultServiceProjectId()) ||
              (await this.getGCEProjectId()) ||
              (await this.getExternalAccountClientProjectId());
            this._cachedProjectId = projectId;
            if (!projectId) {
              throw new Error(
                'Unable to detect a Project Id in the current environment. \n' +
                  'To learn more about authentication and Google APIs, visit: \n' +
                  'https://cloud.google.com/docs/authentication/getting-started'
              );
            }
            resolve(projectId);
          } catch (e) {
            reject(e);
          }
        }
      );
    }

After

this._getDefaultProjectIdPromise = (async () => {
        const projectId =
          this.getProductionProjectId() ||
          (await this.getFileProjectId()) ||
          (await this.getDefaultServiceProjectId()) ||
          (await this.getGCEProjectId()) ||
          (await this.getExternalAccountClientProjectId());
        this._cachedProjectId = projectId;
        if (!projectId) {
          throw new Error(
            'Unable to detect a Project Id in the current environment. \n' +
              'To learn more about authentication and Google APIs, visit: \n' +
              'https://cloud.google.com/docs/authentication/getting-started'
          );
        }
        return projectId;
      })();
    }

shogo-nakano-desu avatar Jul 29 '22 14:07 shogo-nakano-desu

This looks good to me, as long as tests pass as well :) Thank you!

sofisl avatar Aug 02 '22 17:08 sofisl

Thank you. I am going to create PR 👍

shogo-nakano-desu avatar Aug 03 '22 06:08 shogo-nakano-desu

Fixed in https://github.com/googleapis/google-auth-library-nodejs/pull/1436

danielbankhead avatar Aug 23 '22 21:08 danielbankhead