[msal-browser] Adding exports and other changes for extensibility #4459
Conversation
pkanher617
requested review from
jasonnutter,
peterzenz and
tnorling
as code owners
Codecov Report
*This pull request uses carry forward flags. Click here to find out more.
|
| @@ -250,8 +249,6 @@ export abstract class StandardInteractionClient extends BaseInteractionClient { | |||
| } | |||
| } | |||
|
|
|||
| this.browserStorage.updateCacheEntries(validatedRequest.state, validatedRequest.nonce, validatedRequest.authority, validatedRequest.loginHint || "", validatedRequest.account || null); | |||
There was a problem hiding this comment.
Changes in this file, like the RedirectClient.ts look unrelated to the intent of the PR, maybe you need to re-sync with other updates.
There was a problem hiding this comment.
updating cache entries is part of the "extensiblity" changes, those are now done in their respective interaction handlers
| @@ -106,7 +106,8 @@ export enum ApiId { | |||
| export enum InteractionType { | |||
| Redirect = "redirect", | |||
| Popup = "popup", | |||
| Silent = "silent" | |||
| Silent = "silent", | |||
| None = "none" | |||
There was a problem hiding this comment.
This change also looks unrelated to the intent of your PR.
There was a problem hiding this comment.
also related to enums available for downstream packages
| @@ -44,6 +44,7 @@ export class SilentIframeClient extends StandardInteractionClient { | |||
| ...request, | |||
| prompt: PromptValue.NONE | |||
| }, InteractionType.Silent); | |||
| this.browserStorage.updateCacheEntries(silentRequest.state, silentRequest.nonce, silentRequest.authority, silentRequest.loginHint || "", silentRequest.account || null); | |||
There was a problem hiding this comment.
Can we put this in initializeAuthorizationRequest?
Can you also explain what this is for?
There was a problem hiding this comment.
This is to update temporary cache values. Using InitializeAuthorizationRequest makes it harder to control when the cache values are updated. For some broker scenarios, we don't want to update cache entries when we initialize the request, but after we send the request up to the broker.
There was a problem hiding this comment.
I see now that's where it used to be... Can we add a comment explaining this? This feels like something that may get moved back at some point without realizing what it affects. Not sure I like taking a dependency on implementation details like this
There was a problem hiding this comment.
I have the same inclination that this may get "optimized" at some point.
Least case add a comment, Best case make a commented helper API with instructions regarding the sequence ,
processReqBeforeDispatch(browserStorage, request: BaseReqType).
May prevent someone moving this later that way and any other functionality we may add before request dispatch for broker. Could be an overkill, will let you do the trade off.
| @@ -44,6 +44,7 @@ export class SilentIframeClient extends StandardInteractionClient { | |||
| ...request, | |||
| prompt: PromptValue.NONE | |||
| }, InteractionType.Silent); | |||
| this.browserStorage.updateCacheEntries(silentRequest.state, silentRequest.nonce, silentRequest.authority, silentRequest.loginHint || "", silentRequest.account || null); | |||
There was a problem hiding this comment.
nit: Should we replace "" with Constants.EMPTY_STRING?
|
🎉 Handy links: |
This PR adds additional exported files and other changes to further support extensibility in other libraries.