Skip to content
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(clerk-js): Use afterSwitchSessionUrl switching sessions within UserButton #4726

Conversation

panteliselef
Copy link
Member

@panteliselef panteliselef commented Dec 6, 2024

Description

When switching session within <UserButton/>, we should respect the value of afterSwitchSessionUrl. Only inside the SignInAccountSwitcher internal component we need to use afterSignInUrl.

Checklist

  • pnpm test runs as expected.
  • pnpm build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

@panteliselef panteliselef requested review from issuedat and a team December 6, 2024 09:35
@panteliselef panteliselef self-assigned this Dec 6, 2024
Copy link

changeset-bot bot commented Dec 6, 2024

🦋 Changeset detected

Latest commit: 3e3e53d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@clerk/clerk-js Patch
@clerk/chrome-extension Patch
@clerk/clerk-expo Patch

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

Copy link

vercel bot commented Dec 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
clerk-js-sandbox ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 10, 2024 2:34pm

@@ -907,7 +907,7 @@ export class Clerk implements ClerkInterface {
beforeUnloadTracker?.stopTracking();
}

if (redirectUrl) {
if (redirectUrl && !beforeEmit) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clerk.signOut() uses both beforeEmit and redirectUrl which is an incorrect state as the redirection should be trigger by only one of them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we fix signOut? Then throw in this if both beforeEmit and redirectUrl come through.

@panteliselef
Copy link
Member Author

!snapshot

@clerk-cookie
Copy link
Collaborator

Hey @panteliselef - the snapshot version command generated the following package versions:

Package Version
@clerk/astro 2.0.1-snapshot.v20241209201459
@clerk/backend 1.20.3-snapshot.v20241209201459
@clerk/chrome-extension 2.0.9-snapshot.v20241209201459
@clerk/clerk-js 5.40.3-snapshot.v20241209201459
@clerk/elements 0.21.4-snapshot.v20241209201459
@clerk/clerk-expo 2.4.5-snapshot.v20241209201459
@clerk/expo-passkeys 0.0.17-snapshot.v20241209201459
@clerk/express 1.3.26-snapshot.v20241209201459
@clerk/fastify 2.0.28-snapshot.v20241209201459
@clerk/localizations 3.8.2-snapshot.v20241209201459
@clerk/nextjs 6.8.3-snapshot.v20241209201459
@clerk/nuxt 0.0.13-snapshot.v20241209201459
@clerk/clerk-react 5.19.3-snapshot.v20241209201459
@clerk/react-router 0.0.2-snapshot.v20241209201459
@clerk/remix 4.3.6-snapshot.v20241209201459
@clerk/clerk-sdk-node 5.0.77-snapshot.v20241209201459
@clerk/shared 2.19.4-snapshot.v20241209201459
@clerk/tanstack-start 0.6.6-snapshot.v20241209201459
@clerk/testing 1.3.38-snapshot.v20241209201459
@clerk/themes 2.1.55-snapshot.v20241209201459
@clerk/types 4.39.2-snapshot.v20241209201459
@clerk/ui 0.2.4-snapshot.v20241209201459
@clerk/vue 0.0.17-snapshot.v20241209201459

Tip: Use the snippet copy button below to quickly install the required packages.
@clerk/astro

npm i @clerk/[email protected] --save-exact

@clerk/backend

npm i @clerk/[email protected] --save-exact

@clerk/chrome-extension

npm i @clerk/[email protected] --save-exact

@clerk/clerk-js

npm i @clerk/[email protected] --save-exact

@clerk/elements

npm i @clerk/[email protected] --save-exact

@clerk/clerk-expo

npm i @clerk/[email protected] --save-exact

@clerk/expo-passkeys

npm i @clerk/[email protected] --save-exact

@clerk/express

npm i @clerk/[email protected] --save-exact

@clerk/fastify

npm i @clerk/[email protected] --save-exact

@clerk/localizations

npm i @clerk/[email protected] --save-exact

@clerk/nextjs

npm i @clerk/[email protected] --save-exact

@clerk/nuxt

npm i @clerk/[email protected] --save-exact

@clerk/clerk-react

npm i @clerk/[email protected] --save-exact

@clerk/react-router

npm i @clerk/[email protected] --save-exact

@clerk/remix

npm i @clerk/[email protected] --save-exact

@clerk/clerk-sdk-node

npm i @clerk/[email protected] --save-exact

@clerk/shared

npm i @clerk/[email protected] --save-exact

@clerk/tanstack-start

npm i @clerk/[email protected] --save-exact

@clerk/testing

npm i @clerk/[email protected] --save-exact

@clerk/themes

npm i @clerk/[email protected] --save-exact

@clerk/types

npm i @clerk/[email protected] --save-exact

@clerk/ui

npm i @clerk/[email protected] --save-exact

@clerk/vue

npm i @clerk/[email protected] --save-exact

Copy link

@Ephem Ephem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested the snapshot focused on the afterSwitchSessionUrl and it works like a charm! Verified the bug was there before, upgraded and it went away. Found no way to break it. Not quite qualified to review the other changes.

This change will mean that if you relied on afterSignInUrl working before, things will break right (in my testing it did)? I think that's fine and expected considering it's undocumented and does not exist on the types.

redirectUrl: '/after-sign-out',
});
expect(sut.navigate).toHaveBeenCalledWith('/after-sign-out');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was only passing because we were always doing this, inside signOut

const defaultCb = () => this.navigate(redirectUrl);

@panteliselef panteliselef merged commit 85a36a8 into main Dec 12, 2024
28 checks passed
@panteliselef panteliselef deleted the elef/sdki-784-userbutton-not-using-afterswitchsessionurl-on-switching branch December 12, 2024 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants