-
Notifications
You must be signed in to change notification settings - Fork 388
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
feat: Implement support for exposing refreshHandler callback to handle token refresh. #1213
Conversation
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.
One nit about the edit in samples/package.json
, otherwise this looks reasonable to me.
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.
Hey @xil222 I took a first look. I will continue next week. Thanks.
There're 5 test cases:
request():
getAccessToken():
RefreshHandler returns no access token:
Modify other error message and description in tests due to addition of refreshHandler logic. Note: |
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.
The tests look a lot better now. Glad we uncovered this. Thanks for fixing them.
Design Doc: link
For this PR, developer has to provide a callback function returns access_token and expire_time both mandatory.
@bojeil-google @bcoe @lsirac Your feedback is highly appreciated!
There're 8 test cases:
getRequestHeader():
request():
getAccessToken():
RefreshHandler returns no access token:
Modify other error message and description in tests due to addition of refreshHandler logic.
Modify other test cases in request() to add another mock for 2nd successful request.
Note:
Since the logic of throwing error is in refreshTokenNoCache I didn’t add "no refreshHandler callback related information" to the error message in the test case