Skip to content

Conversation

@sthang
Copy link

@sthang sthang commented Oct 21, 2019

No description provided.

@coveralls
Copy link

coveralls commented Oct 21, 2019

Coverage Status

Coverage increased (+0.006%) to 88.068% when pulling f5ca941 on sthang:develop into 6352c75 on intuit:autoRefresh.

@abisalehalliprasan
Copy link
Collaborator

@sthang : Thanks for the PR 👏 . Have a couple of questions with your implementation :

  • I noticed that as part of the createToken method, you are doing a check to see if the autoRefresh is enabled and if YES, the client sets up the autoRefresh. This point forward depending on the autoRefreshIntervalInSeconds field the auto-refresh of tokens goes on until its killed explicitly. But what if there is a call made to OAuthClient.prototype.refresh explicitly ? Will it process the current refresh request into a Queue ?

  • You have updated the sample to stopAutoRefresh on demand as shown here. But the actual stopRefresh() method is missing on the client. were you planning on updating it later?

@sthang
Copy link
Author

sthang commented Oct 21, 2019

@abisalehalliprasan : Thanks for the review.

  1. No. If refresh is called explicitly it will be independent of the autoRefresh and has no impact on the timer. I thought about it and made it independent to keep the implementation simple. The alternative is to reset the timer on setInterval when refresh is called outside of autoRefresh. What do you think?

  2. Here's the stopRefresh on the OAuthClient. The sample app is calling it on the oauthClient.

@abisalehalliprasan
Copy link
Collaborator

@sthang :

1.) We do not want the explicit token refresh call to coincide with the autoRefresh. The workaround seems fine but there is a one-off edge case where the two API calls might coincide(the probability is very low 😃). I will see if I could implement Queue logic here.

Could you close this PR and create a PR for the branch autoRefresh . Apologize for the re-do, I need to test this feature before actually releasing it. Thanks @sthang

@sthang
Copy link
Author

sthang commented Oct 22, 2019

No problem. I've updated the Pull request base to autoRefresh.

@sthang sthang closed this Oct 22, 2019
@sthang sthang reopened this Oct 22, 2019
@sthang sthang changed the base branch from develop to autoRefresh October 22, 2019 04:34
@abisalehalliprasan abisalehalliprasan merged commit 93e6a39 into intuit:autoRefresh Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants