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

Authorization Code Flow for Single Page Applications: Adding Logger class #1155

Merged

Conversation

pkanher617
Copy link
Contributor

This PR adds a logger class to the Authorization Code Flow version of the MSAL library. It also removes instances of console.log from the common package.

Copy link
Contributor

@jasonnutter jasonnutter left a comment

Choose a reason for hiding this comment

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

One suggestion otherwise looks good

lib/msal-common/src/logger/Logger.ts Outdated Show resolved Hide resolved
lib/msal-common/src/logger/Logger.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@jasonnutter jasonnutter left a comment

Choose a reason for hiding this comment

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

One question otherwise looks good.

this.logMessage(message, {
logLevel: LogLevel.Error,
containsPii: false,
correlationId: correlationId || ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Is correlationId used? And I would argue to have this fallback logic to empty string inside of logMessage, so that you don't have to do it in each log level function.

@pkanher617 pkanher617 changed the base branch from authorization-code-flow-login-url to authorization-code-flow-base December 19, 2019 19:03
@pkanher617 pkanher617 merged commit 2c111f2 into authorization-code-flow-base Dec 19, 2019
azure-pipelines bot pushed a commit to pkanher617/microsoft-authentication-library-for-js that referenced this pull request Jan 4, 2022
…logger

Authorization Code Flow for Single Page Applications: Adding Logger class
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants