-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
chore(embedded): refresh the guest token #19132
Conversation
070d315
to
5a481a7
Compare
Codecov Report
@@ Coverage Diff @@
## master #19132 +/- ##
==========================================
- Coverage 66.55% 66.55% -0.01%
==========================================
Files 1646 1646
Lines 63618 63621 +3
Branches 6471 6472 +1
==========================================
Hits 42340 42340
- Misses 19600 19603 +3
Partials 1678 1678
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
LGTM. tested this locally and it worked perfectly 🥳
async function refreshGuestToken() { | ||
const newGuestToken = await fetchGuestToken(); | ||
ourPort.emit('guestToken', { guestToken: newGuestToken }); | ||
setTimeout(refreshGuestToken, getGuestTokenRefreshTiming(newGuestToken)); |
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.
I will take a look on this, but it does not create a recursion that will consume memory on every new timeout? It will free the calling function?
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.
No, there is not a memory leak. Calling setTimeout
schedules a new function call, and the current frame is cleaned up.
// if exp is int, it is in seconds, but Date() takes milliseconds | ||
const exp = new Date(/[^0-9\.]/g.test(parsedJwt.exp) ? parsedJwt.exp : parseFloat(parsedJwt.exp) * 1000); | ||
const isValidDate = exp.toString() !== 'Invalid Date'; | ||
const ttl = isValidDate ? Math.max(MIN_REFRESH_WAIT_MS, exp.getTime() - Date.now()) : DEFAULT_TOKEN_EXP_MS; |
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.
in this case, if (exp - now) is smaller than 5000 couldn't we get some expired notifications?
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.
That is what the Math.max
call is for, we have a minimum refresh time
* refresh the guest token * put back the date logic * version * fix time hijinks * test * Update superset-embedded-sdk/src/guestTokenRefresh.ts (cherry picked from commit 54b60de)
SUMMARY
This refreshes the guest tokens used when embedding a dashboard.
Also fixed guest token generation to use the correct time grain.
relevant: #17187
TESTING INSTRUCTIONS
Embed a dashboard in a demo app (I have been using Preset Manager), wait 5 minutes, then trigger a chart refresh. It should fetch the charts successfully.
ADDITIONAL INFORMATION