Skip to content
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

[msal-browser] Fix Promises in PCA APIs #2091

Merged
merged 4 commits into from Aug 4, 2020
Merged

Conversation

pkanher617
Copy link
Contributor

@pkanher617 pkanher617 commented Aug 4, 2020

Fixes an issue where promises were not resolving in silent flow APIs, which wasn't clearing cache correctly. Should also fix #2089.

@pkanher617 pkanher617 changed the title [msal-browser] Fix Promises PCA APIs [msal-browser] Fix Promises in PCA APIs Aug 4, 2020
@github-actions github-actions bot added msal-browser Related to msal-browser package msal-common Related to msal-common package samples Related to the samples apps for the library. labels Aug 4, 2020
@pkanher617 pkanher617 self-assigned this Aug 4, 2020
const hash = await interactionHandler.monitorPopupForHash(popupWindow, this.config.system.windowHashTimeout);

// Handle response from hash string.
return await interactionHandler.handleCodeResponse(hash);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The catch is a single catch for all issues seen in initialization, creating auth code client and handling the code response. Is that alright?

Copy link
Contributor Author

@pkanher617 pkanher617 Aug 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we cache items in the request preflighting, we need to make sure that we clear it if there are any errors that happen.

Copy link
Collaborator

@tnorling tnorling Aug 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plus cache the error for telemetry purposes. This is done in every API

Copy link
Collaborator

@tnorling tnorling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment about the sample changes but otherwise LGTM

@coveralls
Copy link

coveralls commented Aug 4, 2020

Coverage Status

Coverage increased (+0.2%) to 81.794% when pulling f8e9ee2 on fix-promise-handling-pca into cb1bcbe on dev.

@pkanher617 pkanher617 merged commit 4a94974 into dev Aug 4, 2020
@pkanher617 pkanher617 deleted the fix-promise-handling-pca branch October 2, 2020 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
msal-browser Related to msal-browser package msal-common Related to msal-common package samples Related to the samples apps for the library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Promise returned from "ssoSilent" never resolves or rejects
5 participants