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

[msal-node] Cache-5: Msal node cache additional changes for end-to-end cache to work #1522

Merged
merged 16 commits into from May 6, 2020

Conversation

sameerag
Copy link
Member

This is the final PR in the series of cache PRs and works end-to-end, reads a json file from the disk, retrieves tokens from the server and appends the newly minted ones to the json file in the disk.

Notable changes:

  • NodeStorage update for read/write from the cache
  • Sample update
  • Some tests

Note: Most of the diff for this PR is linter diff; will be trying to minimize this soon by updating the tsdx lint rules to not cause so much disruption for line length which accounts to most changes.

Copy link
Contributor

@sangonzal sangonzal left a comment

Choose a reason for hiding this comment

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

Overall looks good.

Comment on lines +12 to +13
* @param key
* @param value
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: getSerializedCache() doesn't have parameters.

Copy link
Member Author

Choose a reason for hiding this comment

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

Intentional. It is a getter to avoid access to private variables.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove the comments here.

lib/msal-node/src/client/ClientApplication.ts Outdated Show resolved Hide resolved
lib/msal-node/src/client/ClientApplication.ts Outdated Show resolved Hide resolved
lib/msal-node/src/client/ClientApplication.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@pkanher617 pkanher617 left a comment

Choose a reason for hiding this comment

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

Looks ok, I think we need 1 more PR to bring msal-browser up-to-date with these changes before we merge to dev.

@sameerag sameerag merged commit 4c77d1c into msal-node-response May 6, 2020
@sameerag sameerag deleted the msal-node-cache-final branch May 20, 2020 08:06
This was referenced May 20, 2020
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

3 participants