-
Notifications
You must be signed in to change notification settings - Fork 959
Add missing details for Client resource #2994
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
| #### Example | ||
|
|
||
| ```js | ||
| await clerk.client.isNew() |
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.
Is the await here unnecessary or does line 93 need to change to function isNew(): Promise<boolean>?
| #### Example | ||
|
|
||
| ```js | ||
| await clerk.client.clearCache() |
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.
Same as the isNew() comment. Is the await here unnecessary or does line 149 need to change to return a promise?
| Clears any locally cached session data for the current client. | ||
|
|
||
| ```typescript | ||
| function clearCache(): () => void |
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.
function and arrow function here seems wrong, unless clearCache returns a function?
| #### Example | ||
|
|
||
| ```js | ||
| await clerk.client.isEligibleForTouch() |
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.
Same question here. Does it return a boolean or Promise<boolean>?
| Builds a URL that refreshes the current client's authentication state and then redirects the user to the specified URL. | ||
|
|
||
| ```typescript | ||
| function buildTouchUrl(): (params: { redirectUrl: URL }) => string |
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.
Same question here. Are we returning a string or a function that returns a string?
| #### Example | ||
|
|
||
| ```js | ||
| await clerk.client.buildTouchUrl({ redirectUrl: '/' }) |
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.
Does it return a string or Promise<string>?
Co-authored-by: Michael Novotny <manovotny@gmail.com>
|
@manovotny to answer all of your comments, here is what there is in the JS repo:
So the following methods don't return Promises like
Since they're not returning Promises, it makes sense to remove the For the following comment:
|
Let's solve the easy problems first. 😅 If there's no promises, then let's not await. 🤝
These get more complicated.
clearCache(): void {
return this.sessions.forEach(s => s.clearCache());
}There shouldn't be a return statement (we're staying
|




🔎 Previews:
What does this solve?
Linear.
This PR is a follow up from an external contributor PR, that renamed the deprecated
activeSessionsproperty tosignedInSessions.While reviewing that PR, @manovotny and I discovered that the type of
signedInSessionsdidn't seem right. On the JS repo, it shows thatsignedInSessionsis typed asSignedInSession[], but the docs currently list it asSession[].However, after further investigating while working on this ticket, it became clear that
SignedInSession[]is a more specific version ofSession[], narrowing the session type to user-associated sessions with well-defined sign-in states.So what I ended up doing is the following:
signedInSessionsto beSignedInSession[]instead ofSession[], but it still links to theSessionobject page. This is actually already done on this page for e..g: https://clerk.com/docs/nextjs/reference/hooks/use-session.signedInSessionsto be clearer.Clientobject, so documented these as well.