-
Notifications
You must be signed in to change notification settings - Fork 460
fix(clerk-js): Deprecate afterSignOutUrl from UserButton
#3544
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
Changes from all commits
f4f8674
0f1a874
4d98333
062dd5c
31b988d
59e79f3
29a930c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| --- | ||
| '@clerk/clerk-js': minor | ||
| '@clerk/types': minor | ||
| --- | ||
|
|
||
| Deprecate `afterSignOutUrl` and `afterMultiSessionSingleSignOutUrl` from UserButton. | ||
|
|
||
| Developers can now configure these directly in `ClerkProvider` and have them work properly without in UserButton, UserProfile and in impersonation mode. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,15 +4,17 @@ import { useSignOutContext } from '../../contexts'; | |
| import { Col, localizationKeys, Text, useLocalizations } from '../../customizables'; | ||
| import type { FormProps } from '../../elements'; | ||
| import { Form, FormButtons, FormContainer, useCardState, withCardStateProvider } from '../../elements'; | ||
| import { useMultipleSessions } from '../../hooks/useMultipleSessions'; | ||
| import { handleError, useFormControl } from '../../utils'; | ||
|
|
||
| type DeleteUserFormProps = FormProps; | ||
| export const DeleteUserForm = withCardStateProvider((props: DeleteUserFormProps) => { | ||
| const { onReset } = props; | ||
| const card = useCardState(); | ||
| const { navigateAfterSignOut } = useSignOutContext(); | ||
| const { navigateAfterSignOut, navigateAfterMultiSessionSingleSignOutUrl } = useSignOutContext(); | ||
| const { user } = useUser(); | ||
| const { t } = useLocalizations(); | ||
| const { otherSessions } = useMultipleSessions({ user }); | ||
|
|
||
| const confirmationField = useFormControl('deleteConfirmation', '', { | ||
| type: 'text', | ||
|
|
@@ -36,7 +38,12 @@ export const DeleteUserForm = withCardStateProvider((props: DeleteUserFormProps) | |
| } | ||
|
|
||
| await user.delete(); | ||
| await navigateAfterSignOut(); | ||
|
|
||
| // TODO: Investigate if we need to call `setActive` with {session: null} | ||
| if (otherSessions.length === 0) { | ||
| return navigateAfterSignOut(); | ||
| } | ||
| await navigateAfterMultiSessionSingleSignOutUrl(); | ||
|
Comment on lines
+42
to
+46
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💭 I wouldn't necessarily expect that this method would need to be aware of multi-session. I understand why it was done this way as part of this PR, but it feels like a better internal abstraction might be to consolidate the multi-session logic into
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think creating an abstraction here would make our lifer harder, we have
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Benefit would be not having to spread some of this logic across many different methods. That's fine to leave it for now, let's jut keep an eye on these code paths and we can abstract if it feels like multi-session logic is too difficult to manage. |
||
| } catch (e) { | ||
| handleError(e, [], card.setError); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| import { useSessionList } from '@clerk/shared/react'; | ||
| import type { ActiveSessionResource, UserResource } from '@clerk/types'; | ||
|
|
||
| type UseMultipleSessionsParam = { | ||
| user: UserResource | null | undefined; | ||
| }; | ||
|
|
||
| const useMultipleSessions = (params: UseMultipleSessionsParam) => { | ||
| const { sessions } = useSessionList(); | ||
| const activeSessions = sessions?.filter(s => s.status === 'active') as ActiveSessionResource[]; | ||
| const otherSessions = activeSessions.filter(s => s.user?.id !== params.user?.id); | ||
|
|
||
| return { | ||
| activeSessions, | ||
| otherSessions, | ||
| }; | ||
| }; | ||
|
|
||
| export { useMultipleSessions }; |
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.
Do we need to do this investigation before merge?
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 was waiting for #3518 to be merged, and i will check again, but simply calling setActive would not refresh the router.
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.
So i investigated further, even with #3518 the issue persisted. I've identified this as a FAPI issue, because after
user.delete()client_uat comes back with a valid timestamp instead of being0. This will need a separate ticket to be address and should not be part of this PR