-
Notifications
You must be signed in to change notification settings - Fork 327
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
fix(vue): Set initial value of clientCtx
to undefined
#5324
Conversation
🦋 Changeset detectedLatest commit: 7517f9c The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
.changeset/seven-walls-drive.md
Outdated
'@clerk/vue': patch | ||
--- | ||
|
||
fix UsesessionList bug. When `clientCtx` is defined `clerk` can be `null` and throw when trying to access `clerk.value.setActive`. Added a condition to return the default values also when `clerk.value` is `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.
@wobsoriano Clerk being null
while clientCtx
is defined seems like an invalid state. I'd expect the existing code to work as expected, which makes me that there is another underlying cause for this issue.
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.
FYI the issue was client
resource was initially set to an empty object. See my comment here.
This one now looks good!
Hi @Dorilama! Thanks for the PR and reproduction. As @panteliselef mentioned, having While our React In the Vue implementation, we do not use the |
@@ -35,14 +35,14 @@ export const useSessionList: UseSessionList = () => { | |||
const { clerk, clientCtx } = useClerkContext(); | |||
|
|||
const result = computed<UseSessionListReturn>(() => { | |||
if (!clientCtx.value) { | |||
if (!clientCtx.value || !clerk.value) { |
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.
Okay, the issue is that we have the clientCtx
value initially set to an empty object that's why it passes the condition even if clerk
is null.
Would you be able to update the initial value to undefined and assert the type for now? (client: undefined as unknown as ClientResource
). Then we can merge the PR!
This should be a safe update as we check the value of clerk
in all of our composables except useSessionList()
.
Thanks!
clerk.value
in useSessionList()
composable
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.
Thanks so much and very nice catch! 🚀
thank you for all the work with the sdk :) |
.changeset/fuzzy-pillows-doubt.md
Outdated
'@clerk/vue': patch | ||
--- | ||
|
||
updating the initial value of `clientCtx` to `undefined` as requested [here](https://github.com/clerk/javascript/pull/5324#discussion_r1989445357) |
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.
updating the initial value of `clientCtx` to `undefined` as requested [here](https://github.com/clerk/javascript/pull/5324#discussion_r1989445357) | |
Bug fix: Update the initial value of `clientCtx` to `undefined` to correctly infer that `clerk` is defined. |
.changeset/seven-walls-drive.md
Outdated
--- | ||
'@clerk/vue': patch | ||
--- | ||
|
||
fix UsesessionList bug. When `clientCtx` is defined `clerk` can be `null` and throw when trying to access `clerk.value.setActive`. Added a condition to return the default values also when `clerk.value` is `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.
@Dorilama Can you please remove this changeset, in favour of the new one ? We should only have one changeset file per package
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.
yes, sorry about that 😅
clerk.value
in useSessionList()
composableclientCtx
to undefined
Description
Fixes UsesessionList bug: when
clientCtx
is definedclerk
can benull
and the composable throws when trying to accessclerk.value.setActive
.The error can be reproduced in this project on stackblitz
Fixed by adding a condition to return the default values also when
clerk.value
isnull
.Checklist
pnpm test
runs as expected.pnpm build
runs as expected.Type of change