Uh oh!
There was an error while loading. Please reload this page.
fix(clerk-js,types): Close modals when setActive({redirectUrl}) is called - #5092
Conversation
…Active and Clerk.navigate inside UI components
…Active This way we do not need to change the public signature of setActive, or remove the deprecation of beforeEmit, since developers that consume the sdk should still use `redirectUrl`.
…still-mounted-after-delete
🦋 Changeset detectedLatest commit: 8c66b98 The changes in this PR will be included in the next version bump. This PR includes changesets to release 22 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 ↗︎
|
setActive({redirectUrl}) is called| } | ||
| }; | ||
| #resetComponentsState = () => { |
There was a problem hiding this comment.
Wouldn't we still need this for custom flows that mount SignIn/SignUp and call setActive without further navigation?
There was a problem hiding this comment.
I am a bit confused by the question, you can either have a custom flow, or mount SignIn/Up, right ? Can you give a n example when this is not the case ?
There was a problem hiding this comment.
You're right, let me rephrase in another way then:
If I understood correctly, the modals get closed upon router navigation after setActive - but what happens if there's no navigation, you're just calling regular setActive - wouldn't we want to keep this current logic for closing SignIn / SignUp?
Also, setActive inner workings is still relately new to me as I'll be pushing some changes to it soon, so I'm mostly asking for my own understanding here 🫡
There was a problem hiding this comment.
#resetComponentsStatedoes not cover all modals which is weird on it's own.- Having
#resetComponentsStatethere, cause an open SignInModal to close if for whatever reason asetActive({session: null})is called in a multi session app. Another weird side effect.
wouldn't we want to keep this current logic for closing SignIn / SignUp?
Our components will always redirect to afterSignIn or afterSignUp, which means we make a navigation outside the component so the modal will close.
The only modal component that exists today that does not navigate on setActive, isReverificationand it closes the modal manually (insideuseAfterVerification).
Are we aligned that we care about closing the modal, only as long setActive is called within our components ? And as long as we always setActive({redirectUrl}) from within our component at the end of each flow the modals will close properly.
Removing #resetComponentsState might unblock afterAuth in a way, because now you can call setActive (without redirectUrl) from /sign-in/select-org/ and then internally navigate to /sign-in/subscribe and not having the modal close on you.
If you have scenarios that seem problematic with the changes that this PR proposes, please let us know as we might need to switch back to the more complex solution which allows for each component to close their own modal similarly to how we had setActive working prior to the deprecation of beforeEmit.
There was a problem hiding this comment.
Makes sense to me! I'll make sure to consider that behavior when testing after-auth changes, and we also discuss further about that, thanks for handling those cases 🫡
LauraBeatris
left a comment
There was a problem hiding this comment.
Just missing the changeset description. Other than that, changes look good to me! 🫡
Uh oh!
There was an error while loading. Please reload this page.
| expect(sessionCookieList.length).toBe(0); | ||
| }); | ||
| test('closes the modal after delete', async ({ page, context }) => { |
There was a problem hiding this comment.
Can you also add a test that covers the removed resetComponentState functionality? So closing a sign-in modal on sign in.
Uh oh!
There was an error while loading. Please reload this page.
| return () => { | ||
| unsubscribe(); | ||
| }; | ||
| // We are not expecting `onExternalNavigate` to change |
There was a problem hiding this comment.
I know we're not expecting it to change, but does providing it as a dependency break anything?
There was a problem hiding this comment.
since we pass a non memoize function i think it will cause multiple subscriptions/unsubscribes
Uh oh!
There was an error while loading. Please reload this page.
| testAgainstRunningApps({ withEnv: [appConfigs.envs.withEmailCodes] })('sign up flow @generic @nextjs', ({ app }) => { | ||
| test.describe.configure({ mode: 'serial' }); | ||
| test.describe.configure({ mode: 'parallel' }); |
There was a problem hiding this comment.
Seems to improve speed and does not cause tests to fail
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Dylan Staley <88163+dstaley@users.noreply.github.com>
panteliselef
commented
Feb 12, 2025
!snapshot |
clerk-cookie
commented
Feb 12, 2025
Hey @panteliselef - the snapshot version command generated the following package versions:
Tip: Use the snippet copy button below to quickly install the required packages. npm i @clerk/astro@2.2.0-snapshot.v20250212105537 --save-exact
npm i @clerk/backend@1.24.1-snapshot.v20250212105537 --save-exact
npm i @clerk/chrome-extension@2.2.9-snapshot.v20250212105537 --save-exact
npm i @clerk/clerk-js@5.52.3-snapshot.v20250212105537 --save-exact
npm i @clerk/elements@0.22.22-snapshot.v20250212105537 --save-exact
npm i @clerk/clerk-expo@2.7.7-snapshot.v20250212105537 --save-exact
npm i @clerk/expo-passkeys@0.1.20-snapshot.v20250212105537 --save-exact
npm i @clerk/express@1.3.48-snapshot.v20250212105537 --save-exact
npm i @clerk/fastify@2.1.21-snapshot.v20250212105537 --save-exact
npm i @clerk/localizations@3.10.6-snapshot.v20250212105537 --save-exact
npm i @clerk/nextjs@6.11.3-snapshot.v20250212105537 --save-exact
npm i @clerk/nuxt@1.1.4-snapshot.v20250212105537 --save-exact
npm i @clerk/clerk-react@5.22.13-snapshot.v20250212105537 --save-exact
npm i @clerk/react-router@1.0.8-snapshot.v20250212105537 --save-exact
npm i @clerk/remix@4.4.24-snapshot.v20250212105537 --save-exact
npm i @clerk/shared@2.21.1-snapshot.v20250212105537 --save-exact
npm i @clerk/tanstack-start@0.9.6-snapshot.v20250212105537 --save-exact
npm i @clerk/testing@1.4.22-snapshot.v20250212105537 --save-exact
npm i @clerk/themes@2.2.18-snapshot.v20250212105537 --save-exact
npm i @clerk/types@4.46.0-snapshot.v20250212105537 --save-exact
npm i @clerk/ui@0.3.23-snapshot.v20250212105537 --save-exact
npm i @clerk/vue@1.1.12-snapshot.v20250212105537 --save-exact |
be5c2ae to
8c66b98Compare
Description
A simpler solution to #5077 which allows the e2e test of #5071 to pass 🟢.
General Problem
Since the deprecation of
setActive({ beforeEmit }), our UI components have been usingClerk.navigatein order to navigate whensetActive({ redirectUrl })is set which does no handle closing the already open modals.Specific use-case
This buggy behaviour is clearly visible, when you attempt to delete a user from UserProfile rendered in a modal. Upon deletion,
setActive({redirectUrl})was called, but when a soft navigation occurred (using framework router) the modal would not close, and the backdrop was clearly visible.Solution
This solution in this PR fixes the above issues while maintaining
setActivesimple to use for developers. We also still discourage the explicit usage ofsetActive({beforeEmit})Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change