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

Add logs to msal-browser #3004

Merged
merged 15 commits into from Mar 2, 2021
Merged

Add logs to msal-browser #3004

merged 15 commits into from Mar 2, 2021

Conversation

jo-arroyo
Copy link
Collaborator

@jo-arroyo jo-arroyo commented Feb 10, 2021

This PR:

  • Adds log messages to BrowserCacheManager.ts
  • Updates log messages in RedirectHandler.ts

@github-actions github-actions bot added the msal-browser Related to msal-browser package label Feb 10, 2021
@coveralls
Copy link

coveralls commented Feb 10, 2021

Coverage Status

Coverage decreased (-0.08%) to 82.633% when pulling 7f21fd7 on browser-add-logs into 0104fa2 on dev.

@jo-arroyo jo-arroyo marked this pull request as ready for review March 1, 2021 18:25
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 non-blocking comment. Can be done incrementally if that makes more sense.

@@ -288,6 +295,7 @@ export class BrowserCacheManager extends CacheManager {
* @param appMetadataKey
*/
getAppMetadata(appMetadataKey: string): AppMetadataEntity | null {
this.logger.verbose("BrowserCacheManager.getAppMetadata called");
const value = this.getItem(appMetadataKey);
if (!value) {
return null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

In each of the getters should we also log whether or not we successfully found something in the cache?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense. I added cache hit logs, and it makes the logs a lot longer. If we discover it creates too much noise, I'm happy to remove them later.

…7.json

Co-authored-by: Thomas Norling <thomas.l.norling@gmail.com>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants