-
Notifications
You must be signed in to change notification settings - Fork 364
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
Changes to logout and cache synchronicity #758
Conversation
This pull request introduces 1 alert when merging c31598a into ecafb37 - view on LGTM.com new alerts:
|
window.location.assign(url); | ||
}; | ||
|
||
if (this.options.cache) { |
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.
Perhaps we should do:
const maybePromise = this.cacheManager.clear();
if (typeof maybePromise?.then === 'function') {
return maybePromise.then(postCacheClear)
}
postCacheClear();
Then the advice would be If you provide an async cache ...
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.
Sounds reasonable!
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.
Diving into this, cacheManager.clear
will always return a Promise; are you also suggesting removing cacheManager.clearSync
?
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.
D'oh - no sorry, I forgot about clear/clearSync ignore me
Description
This PR makes the following changes around logout and caching:
logout
can now optionally return a Promise. This is currently the case when a custom cache implementation is specified using thecache
optionlogout
acts synchronously and does not return a promiseICache
methods (get
,set
,remove
,allKeys
) can now optionally return a promise or a static value, and is handled seamlesslyPromise.resolve
The motivation was for SDKs like
auth0-angular
andauth0-react
who update their internal state reactively can now rely on logout being synchronous by default, and update their state accordingly.Note that none of this should be a breaking change for consumers:
logout
never returned a value so there was nothing to handleTesting
Checklist
master