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 dependencies in AuthenticationHeaderParser and AsyncMemoryStorage #4235

Merged
merged 7 commits into from
Nov 3, 2021

Conversation

hectormmg
Copy link
Member

This PR:

  • Changes the ClientAuthError import in AuthenticationHeaderParser from "../index.ts" to "../error/ClientAuthError.ts"
  • Changes the BrowserAuthError import in AsyncMemoryStorage from "../index.ts" to "../error/BrowserAuthError"
  • Changes the Logger import in AsyncMemoryStorage from "../index.ts" to "@azure/msal-common"

@github-actions github-actions bot added msal-browser Related to msal-browser package msal-common Related to msal-common package labels Nov 2, 2021
@github-actions github-actions bot removed msal-browser Related to msal-browser package msal-common Related to msal-common package labels Nov 2, 2021
@jasonnutter
Copy link
Contributor

Interesting that typescript doesn't catch this...is this breaking the library right now?

@hectormmg
Copy link
Member Author

Interesting that typescript doesn't catch this...is this breaking the library right now?

@jasonnutter not just that, the circular dependency was actually added by VS Code's "Quick fix..." import option

@github-actions github-actions bot added msal-browser Related to msal-browser package msal-common Related to msal-common package labels Nov 2, 2021
@tnorling
Copy link
Collaborator

tnorling commented Nov 3, 2021

Looks like there's a lint rule we could use to catch this in the future: https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-cycle.md

@codecov-commenter
Copy link

Codecov Report

Merging #4235 (4faf8f9) into dev (13a7548) will not change coverage.
The diff coverage is 100.00%.

Flag Coverage Δ *Carryforward flag
msal-angular 96.39% <ø> (ø)
msal-browser 87.77% <100.00%> (ø)
msal-common 85.41% <100.00%> (ø)
msal-core 77.07% <ø> (ø) Carriedforward from 13a7548
msal-node 81.93% <ø> (ø)
msal-react 94.19% <ø> (ø)

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

Impacted Files Coverage Δ
lib/msal-browser/src/cache/AsyncMemoryStorage.ts 68.18% <100.00%> (ø)
...l-common/src/request/AuthenticationHeaderParser.ts 100.00% <100.00%> (ø)

@hectormmg hectormmg merged commit e983e25 into dev Nov 3, 2021
@hectormmg hectormmg deleted the remove-circular-dependencies branch November 3, 2021 19:31
@ghost
Copy link

ghost commented Dec 7, 2021

🎉@azure/msal-browser@v2.20.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link

ghost commented Dec 7, 2021

🎉@azure/msal-common@v5.2.0 has been released which incorporates this pull request.:tada:

We recommend upgrading to the latest version of @azure/msal-browser or @azure/msal-node to take advantage of this change.

Handy links:

pkanher617 pushed a commit to pkanher617/microsoft-authentication-library-for-js that referenced this pull request Dec 30, 2021
…ryStorage (AzureAD#4235)

* Change files

* Re-implement circular dependency fix
azure-pipelines bot pushed a commit to pkanher617/microsoft-authentication-library-for-js that referenced this pull request Jan 4, 2022
…ryStorage (AzureAD#4235)

* Change files

* Re-implement circular dependency fix
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants