-
Notifications
You must be signed in to change notification settings - Fork 409
Using the new HttpClient API in Credentials #318
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
bojeil-google
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just some nits while you are already making these changes.
src/auth/credential.ts
Outdated
| */ | ||
| export class CertCredential implements Credential { | ||
| private certificate_: Certificate; | ||
| private readonly certificate_: Certificate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since you're already modifying this, let's remove the trailing underscore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/auth/credential.ts
Outdated
| */ | ||
| export class RefreshTokenCredential implements Credential { | ||
| private refreshToken_: RefreshToken; | ||
| private readonly refreshToken_: RefreshToken; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Let's remove trailing underscore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Using the new
HttpClientAPI in Credentials module. With this I believe we are using the new API everywhere in the SDK (I will create a separate PR to remove the old code). I also managed to re-enable an old refresh token unit test that had been disabled for a long time.This is a follow up to #280, #291, and #301