-
Notifications
You must be signed in to change notification settings - Fork 32
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
[MOB-6063] updates parameter types for setEmail and setUserId #491
[MOB-6063] updates parameter types for setEmail and setUserId #491
Conversation
ts/Iterable.ts
Outdated
* @param {string | undefined} authToken valid, pre-fecthed JWT the SDK can use to authenticate API requests, optional - if null/undefined, no JWT related action will be taken | ||
*/ | ||
|
||
static setEmail(email: string | undefined, authToken?: string | undefined) { | ||
static setEmail(email: string | null, authToken?: string | null) { |
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.
I think @pauljung14 suggested to have undefined AND null both.
so it will be string | undefined | null
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.
You can find my suggestion in the comments of the JIRA ticket
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.
Ah gotcha. I was thinking that we wanted the email to be required. But, I will make the change.
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.
I think we need to verify what behavior occurs when the user is undefined. Are we logging out the user?
* @param {string | undefined} authToken valid, pre-fecthed JWT the SDK can use to authenticate API requests, optional - if null/undefined, no JWT related action will be taken | ||
*/ | ||
|
||
static setEmail(email: string | undefined, authToken?: string | undefined) { | ||
static setEmail(email?: string | null, authToken?: string | null) { |
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.
The null and undefined changes will go here
* optional parameter: @param {string | undefined} authToken valid, pre-fecthed JWT the SDK can use to authenticate API requests, optional - if null/undefined, no JWT related action will be taken | ||
*/ | ||
|
||
static setUserId(userId: string | undefined, authToken?: string | undefined) { |
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.
And here
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.
lgtm!
🔹 JIRA Ticket(s) if any
✏️ Description
This pull request updates the parameter types for setEmail and setUserId. This stems from reported issues with setEmail as discussed here. In our documentation, we ask customers to pass in a value of null to log out.