-
Notifications
You must be signed in to change notification settings - Fork 208
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
[SDK-3534] Credential manager support #497
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, left a couple comments 🎉
* @param {Number} credentials.expiresIn optional - `expiresAt` should not be empty if this is. Used to denote when the token will expire from the issued time | ||
* @param {String} credentials.expiresAt optional - `expiresIn` should not be empty if this is. Used to denote when the token will expire. Has precendence over `expiresIn`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure on the docs here. Looking at the code, either expiresIn
or expiresAt
is required but at least one of them must be specified, but the docs say they're both optional.
Edit: ok I see now:
credentials.expiresIn optional -
expiresAt
should not be empty if this is.
I found this really confusing and we should reword. How about:
"credentials.expiresIn - optional if expiresAt
is set.."
And the converse for expiresAt
.
} | ||
|
||
//private | ||
async ensureCredentialManagerIsInitialized() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we prefix this?
i.e. _ensureCredentialManagerIsInitialized
Will we be adding tests here or in a follow-up PR? I'm not seeing any in the diff. |
Co-authored-by: Steve Hobbs <steve.hobbs.mail@gmail.com>
Co-authored-by: Steve Hobbs <steve.hobbs.mail@gmail.com>
Changes
We have provided Credentials Manager support for Android in React Native. The following methods are added
References
#476
Testing
Checklist