[core-rest-pipeline] Token refresher update, based on the latest design by Will#14554
Conversation
xirzec
left a comment
xirzec
left a comment
There was a problem hiding this comment.
I think this looks good to me. I confess I didn't think super hard about the cycler implementation as I assume it's unchanged from core-http
| async function beginRefresh( | ||
| getAccessToken: () => Promise<AccessToken | null>, | ||
| retryIntervalInMs: number, | ||
| timeoutInMs: number |
There was a problem hiding this comment.
nit: timeoutInMs really sounds like the number of ms to wait, not a date stamp in epoch ticks. Can we rename to something better? I'm struggling to come up with it though... /cc @bterlson any thoughts?
There was a problem hiding this comment.
What about refreshExpiry? @willmtemple
I'm ok with other names, but I agree with Jeff.
There was a problem hiding this comment.
I’ll use refreshExpiry for now, Brian liked it over teams!
There was a problem hiding this comment.
Wait I like refreshTimeout more... I'll push refresh timeout for now
There was a problem hiding this comment.
I need to fix conflicts on another PR for Jeremy, I'll merge this for now. We can address more feedback alter 😁😁
|
Hello @sadasant! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
3 participants
@willmtemple just finished a rewrite of the bearerTokenAuthenticationPolicy on the old core: PR #14223
We also want to have this same update in the new core, so I've copied the changes he did into core-rest-pipeline.
This PR closes an early draft I did some time ago: #13832
For this PR I've decided, similar to Will's PR on the old core, to not alter the public API of
core-rest-pipeline. We can keep this refresher configuration private until customers ask for this. These are also private in other languages.Feedback always appreciated 🙏
Related to #13369