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-browser][msal-common] Make library state an object and add timestamp (Updated) #1790

Merged
merged 9 commits into from Jun 23, 2020

Conversation

pkanher617
Copy link
Contributor

This PR is an update of #1675, made because the cache update drastically changed the surface of the library. The changes made in this PR will be merged, and #1675 will be closed.

This PR converts library state from a GUID to a stringified JSON object with id and timestamp fields. This will allow msal-common and msal-browser to handle situations where responses are handled long after the request was initiated, and setting correct token expiration lifetimes.

Also includes tests.

@github-actions github-actions bot added msal-browser Related to msal-browser package msal-common Related to msal-common package labels Jun 17, 2020
Copy link
Collaborator

@tnorling tnorling left a comment

Choose a reason for hiding this comment

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

Generally looks good, just missing tests

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.

A few changes requested, agree about tests.

@jasonnutter
Copy link
Contributor

@pkanher617 pkanher617 changed the base branch from msal-common-account-interface to dev June 23, 2020 16:27
Copy link
Collaborator

@tnorling tnorling left a comment

Choose a reason for hiding this comment

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

One nitpick but otherwise LGTM

@@ -932,7 +959,7 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
"tid": "3338040d-6c67-4c5b-b112-36a304b66dad",
"nonce": "123523",
};
const testAccount: IAccount = {
const testAccount: AccountInfo = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like we recreate this testAccount in a lot of places. Can we just declare this at the top of the file or in a utils file once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will optimize tests in a later PR.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 77.587% when pulling 9894c0f on msal-2-state-changes into f842023 on dev.

Copy link
Contributor

@jmckennon jmckennon 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 to me

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.

LGTM. @sameerag to follow up on the impact if state is supported in node

@pkanher617 pkanher617 merged commit ee6a06b into dev Jun 23, 2020
@pkanher617 pkanher617 deleted the msal-2-state-changes branch August 1, 2020 00:07
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

6 participants