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

Refactor token cache entities into types #6580

Merged
merged 7 commits into from
Oct 24, 2023
Merged

Conversation

tnorling
Copy link
Collaborator

@tnorling tnorling commented Oct 19, 2023

Refactors CredentialEntity, IdTokenEntity, AccessTokenEntity, and RefreshTokenEntity to be Types rather than a Class and moves static class methods into functions exported to the CacheHelpers namespace.

Making these types and separating the class methods from the type definition allows us to read from the cache and directly use the value without needing to first copy each key/value pair into an instance of the class (the toObject helper function). Doing it this way also results in a small bundle size improvement.

Reviews can be focused on the msal-common/src/cache folder. The rest of the changes are repetitively changing references to the affected functions

@github-actions github-actions bot added msal-node Related to msal-node package msal-browser Related to msal-browser package msal-common Related to msal-common package labels Oct 19, 2023
@codecov-commenter
Copy link

codecov-commenter commented Oct 19, 2023

Codecov Report

Merging #6580 (cffde12) into dev (81d34b4) will decrease coverage by 4.13%.
Report is 361 commits behind head on dev.
The diff coverage is 60.00%.

Flag Coverage Δ *Carryforward flag
msal-angular 96.73% <ø> (+0.22%) ⬆️ Carriedforward from 93487d6
msal-browser 79.00% <ø> (-7.47%) ⬇️ Carriedforward from 93487d6
msal-common 83.97% <ø> (-0.58%) ⬇️
msal-core ?
msal-node 80.72% <ø> (-2.67%) ⬇️ Carriedforward from 93487d6
msal-node-extensions 67.37% <59.74%> (-8.27%) ⬇️ Carriedforward from 93487d6
msal-react 94.24% <ø> (-0.45%) ⬇️ Carriedforward from 93487d6
node-token-validation ?

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

Files Coverage Δ
extensions/msal-node-extensions/src/Dpapi.ts 100.00% <100.00%> (ø)
.../msal-node-extensions/src/error/NativeAuthError.ts 100.00% <100.00%> (ø)
extensions/msal-node-extensions/src/index.ts 100.00% <100.00%> (ø)
...nsions/msal-node-extensions/src/packageMetadata.ts 100.00% <100.00%> (+100.00%) ⬆️
...-extensions/src/persistence/DataProtectionScope.ts 100.00% <100.00%> (ø)
...nsions/msal-node-extensions/src/utils/Constants.ts 100.00% <100.00%> (ø)
...sions/msal-node-extensions/src/utils/TypeGuards.ts 100.00% <100.00%> (ø)
lib/msal-angular/src/constants.ts 100.00% <ø> (ø)
lib/msal-angular/src/msal.broadcast.service.ts 100.00% <ø> (ø)
lib/msal-angular/src/msal.guard.ts 90.78% <ø> (+0.64%) ⬆️
... and 70 more

... and 180 files with indirect coverage changes

Copy link
Member

@hectormmg hectormmg left a comment

Choose a reason for hiding this comment

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

Looks good. Is there a reason why we're not doing the same for AccountEntity?

@tnorling
Copy link
Collaborator Author

Looks good. Is there a reason why we're not doing the same for AccountEntity?

I will do the other entities in separate PR(s). Was trying to keep this as small as possible, but it ended up pretty big anyway :/

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.

Some small comments.

@tnorling tnorling enabled auto-merge (squash) October 24, 2023 19:42
@tnorling tnorling merged commit 5f49a5e into dev Oct 24, 2023
45 of 46 checks passed
@tnorling tnorling deleted the refactor-TokenEntities branch October 24, 2023 19:50
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 msal-node Related to msal-node package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants