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

Fix circular dependency #5402

Merged
merged 3 commits into from
Nov 21, 2022
Merged

Fix circular dependency #5402

merged 3 commits into from
Nov 21, 2022

Conversation

tnorling
Copy link
Collaborator

Fixes circular dependency

@codecov-commenter
Copy link

Codecov Report

Merging #5402 (9b24f04) into dev (f318796) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Flag Coverage Δ *Carryforward flag
msal-angular 96.50% <ø> (ø) Carriedforward from f318796
msal-browser 86.47% <ø> (ø) Carriedforward from f318796
msal-common 85.37% <ø> (ø) Carriedforward from f318796
msal-core 82.84% <ø> (ø) Carriedforward from f318796
msal-node 83.33% <100.00%> (-0.04%) ⬇️
msal-node-extensions 76.03% <ø> (ø) Carriedforward from f318796
msal-react 94.61% <ø> (ø) Carriedforward from f318796
node-token-validation 88.88% <ø> (ø) Carriedforward from f318796

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
lib/msal-node/src/utils/NetworkUtils.ts 100.00% <ø> (ø)
lib/msal-node/src/config/Configuration.ts 100.00% <100.00%> (ø)

Copy link
Contributor

@derisen derisen left a comment

Choose a reason for hiding this comment

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

LGTM -not sure if needs action but the same pattern seems to exist in node-token-validation as well

@@ -122,7 +122,7 @@ const DEFAULT_LOGGER_OPTIONS: LoggerOptions = {

const DEFAULT_SYSTEM_OPTIONS: Required<NodeSystemOptions> = {
loggerOptions: DEFAULT_LOGGER_OPTIONS,
networkClient: NetworkUtils.getNetworkClient(),
networkClient: new HttpClient(),
Copy link
Member

Choose a reason for hiding this comment

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

I thought the initial idea was to ensure generic client setup, where setNetworkClient() would enable them to set the specific network client.

Copy link
Member

@sameerag sameerag left a comment

Choose a reason for hiding this comment

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

Approved. I think we already have a demo by Robbie for custom network implementation, so this looks fine.

@tnorling tnorling enabled auto-merge (squash) November 21, 2022 18:57
@tnorling tnorling merged commit a3960d5 into dev Nov 21, 2022
@tnorling tnorling deleted the fix-circular-dependency branch November 21, 2022 19:01
@ghost
Copy link

ghost commented Nov 22, 2022

🎉@azure/msal-node@v1.14.4 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
msal-node Related to msal-node package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants