[msal-common][msal-node][#4] Update UnifiedCacheManager.ts #1771
Conversation
pkanher617
requested review from
DarylThayil,
jasonnutter,
jmckennon,
jo-arroyo,
sameerag,
hectormmg and
tnorling
as code owners
github-actions
bot
added
msal-browser
pkanher617
changed the title
There was a problem hiding this comment.
I don't think we should:
- Have getAccounts be an API on BaseClient
- Make all APIs in UnifiedCacheManager be statics
My understanding is that these changes were made because getAccounts and the unifiedCacheManager APIs were not accessible from node.
I think the proper solution to expose getAllAccounts to Node is to construct UnifiedCacheManager in ClientApplication in Node and in PublicClientApplication in browser, and pass that in as part of the BaseClient configuration.
getAccounts and removeAccount can then be part of cacheManager, where they logically belong, and removes the need to make all APIs in UnifiedCacheManager static.
| return this.getAccountsFilteredBy(); | ||
| getAllAccounts(): IAccount[] { | ||
| const currentAccounts: AccountCache = this.getAccountsFilteredBy(); | ||
| const accountValues: AccountEntity[] = Object.values(currentAccounts); |
There was a problem hiding this comment.
Object.values won't work in IE11 afaik. I think there's already a lot of incompatibility in 2.0 so this won't be the thing that breaks it, but its more to clean up later if/when we do work on compatibility.
There was a problem hiding this comment.
good call. @pkanher617 can you please note this to track when we do a recon on msal-common?
| } | ||
| } | ||
|
|
||
| export class DefaultStorageClass extends CacheManager { |
There was a problem hiding this comment.
All default implementation are in ClientConfiguration. Seems like this should also go in there?
There was a problem hiding this comment.
I put this here because if the interface ever changes, you can make all the changes in a single file instead of having to jump between here and ClientConfiguration.ts
| @@ -33,17 +28,11 @@ export abstract class BaseClient { | |||
| protected cryptoUtils: ICrypto; | |||
|
|
|||
| // Storage Interface | |||
| protected cacheStorage: ICacheStorage; | |||
| protected cacheStorage: CacheManager; | |||
There was a problem hiding this comment.
We should either rename CacheManager to CacheStorage, or rename the variables to match the class name.
There was a problem hiding this comment.
I can rename the variable to cacheManager.
This PR does the following:
msal-node