From 2c0b9012e750c6ba0463564396d006afce5b71a0 Mon Sep 17 00:00:00 2001 From: Bryce Kalow Date: Mon, 3 Jun 2024 15:47:49 -0500 Subject: [PATCH 1/6] Fix issue when passing redirect props to SignInButton and SignOutButton --- .../next-app-router/src/app/buttons/page.tsx | 21 ++++++ integration/testUtils/commonPageObject.ts | 2 +- integration/tests/redirects.test.ts | 66 +++++++++++++++++++ .../react/src/components/SignInButton.tsx | 11 ++-- .../react/src/components/SignUpButton.tsx | 11 ++-- 5 files changed, 102 insertions(+), 9 deletions(-) create mode 100644 integration/templates/next-app-router/src/app/buttons/page.tsx create mode 100644 integration/tests/redirects.test.ts diff --git a/integration/templates/next-app-router/src/app/buttons/page.tsx b/integration/templates/next-app-router/src/app/buttons/page.tsx new file mode 100644 index 0000000000..0755a45c48 --- /dev/null +++ b/integration/templates/next-app-router/src/app/buttons/page.tsx @@ -0,0 +1,21 @@ +import { SignInButton } from '@clerk/nextjs'; + +export default function Home() { + return ( +
+ + Sign in button (force) + + + + Sign in button (fallback) + +
+ ); +} diff --git a/integration/testUtils/commonPageObject.ts b/integration/testUtils/commonPageObject.ts index 3243a35748..352a143465 100644 --- a/integration/testUtils/commonPageObject.ts +++ b/integration/testUtils/commonPageObject.ts @@ -3,7 +3,7 @@ import type { TestArgs } from './signInPageObject'; export const common = ({ page }: TestArgs) => { const self = { continue: () => { - return page.getByRole('button', { name: 'Continue', exact: true }).click(); + return page.locator('button').filter({ hasText: 'Continue' }).click(); }, setPassword: (val: string) => { return page.locator('input[name=password]').fill(val); diff --git a/integration/tests/redirects.test.ts b/integration/tests/redirects.test.ts new file mode 100644 index 0000000000..50ef366cb8 --- /dev/null +++ b/integration/tests/redirects.test.ts @@ -0,0 +1,66 @@ +import { test } from '@playwright/test'; + +import { appConfigs } from '../presets'; +import type { FakeUser } from '../testUtils'; +import { createTestUtils, testAgainstRunningApps } from '../testUtils'; + +testAgainstRunningApps({ withEnv: [appConfigs.envs.withEmailCodes] })('redirect props @nextjs', ({ app }) => { + test.describe.configure({ mode: 'serial' }); + + let fakeUser: FakeUser; + + test.beforeAll(async () => { + const u = createTestUtils({ app }); + fakeUser = u.services.users.createFakeUser({ + fictionalEmail: true, + withPhoneNumber: true, + withUsername: true, + }); + await u.services.users.createBapiUser(fakeUser); + }); + + test.afterAll(async () => { + await fakeUser.deleteIfExists(); + await app.teardown(); + }); + + test.afterEach(async ({ page, context }) => { + const u = createTestUtils({ app, page, context }); + await u.page.signOut(); + await u.page.context().clearCookies(); + }); + + test('sign in button respects forceRedirectUrl', async ({ page, context }) => { + const u = createTestUtils({ app, page, context }); + + await u.page.goToRelative('/buttons'); + await u.page.waitForClerkJsLoaded(); + await u.po.expect.toBeSignedOut(); + + await u.page.getByText('Sign in button (force)').click(); + + await u.po.signIn.waitForMounted(); + await u.po.signIn.signInWithEmailAndInstantPassword({ email: fakeUser.email, password: fakeUser.password }); + + await u.page.waitForAppUrl('/protected'); + + await u.po.expect.toBeSignedIn(); + }); + + test('sign in button respects fallbackRedirectUrl', async ({ page, context }) => { + const u = createTestUtils({ app, page, context }); + + await u.page.goToRelative('/buttons'); + await u.page.waitForClerkJsLoaded(); + await u.po.expect.toBeSignedOut(); + + await u.page.getByText('Sign in button (fallback)').click(); + + await u.po.signIn.waitForMounted(); + await u.po.signIn.signInWithEmailAndInstantPassword({ email: fakeUser.email, password: fakeUser.password }); + + await u.page.waitForAppUrl('/protected'); + + await u.po.expect.toBeSignedIn(); + }); +}); diff --git a/packages/react/src/components/SignInButton.tsx b/packages/react/src/components/SignInButton.tsx index 50cec831de..796f68220f 100644 --- a/packages/react/src/components/SignInButton.tsx +++ b/packages/react/src/components/SignInButton.tsx @@ -1,3 +1,4 @@ +import type { SignInProps } from '@clerk/types'; import React from 'react'; import type { SignInButtonProps, WithClerkProp } from '../types'; @@ -11,11 +12,11 @@ export const SignInButton = withClerk(({ clerk, children, ...props }: WithClerkP const child = assertSingleChild(children)('SignInButton'); const clickHandler = () => { - const opts = { + const opts: SignInProps = { + forceRedirectUrl, + fallbackRedirectUrl, signUpFallbackRedirectUrl, signUpForceRedirectUrl, - signInForceRedirectUrl: forceRedirectUrl, - signInFallbackRedirectUrl: fallbackRedirectUrl, }; if (mode === 'modal') { @@ -25,7 +26,9 @@ export const SignInButton = withClerk(({ clerk, children, ...props }: WithClerkP }; const wrappedChildClickHandler: React.MouseEventHandler = async e => { - await safeExecute((child as any).props.onClick)(e); + if (child && typeof child === 'object' && 'props' in child) { + await safeExecute(child.props.onClick)(e); + } return clickHandler(); }; diff --git a/packages/react/src/components/SignUpButton.tsx b/packages/react/src/components/SignUpButton.tsx index 1db1a5f30e..8a212fe5f9 100644 --- a/packages/react/src/components/SignUpButton.tsx +++ b/packages/react/src/components/SignUpButton.tsx @@ -1,3 +1,4 @@ +import type { SignUpProps } from '@clerk/types'; import React from 'react'; import type { SignUpButtonProps, WithClerkProp } from '../types'; @@ -19,9 +20,9 @@ export const SignUpButton = withClerk(({ clerk, children, ...props }: WithClerkP const child = assertSingleChild(children)('SignUpButton'); const clickHandler = () => { - const opts = { - signUpFallbackRedirectUrl: fallbackRedirectUrl, - signUpForceRedirectUrl: forceRedirectUrl, + const opts: SignUpProps = { + fallbackRedirectUrl, + forceRedirectUrl, signInFallbackRedirectUrl, signInForceRedirectUrl, unsafeMetadata, @@ -35,7 +36,9 @@ export const SignUpButton = withClerk(({ clerk, children, ...props }: WithClerkP }; const wrappedChildClickHandler: React.MouseEventHandler = async e => { - await safeExecute((child as any).props.onClick)(e); + if (child && typeof child === 'object' && 'props' in child) { + await safeExecute(child.props.onClick)(e); + } return clickHandler(); }; From 2a293aff2f393e936c400271cc1b229ae098d6e6 Mon Sep 17 00:00:00 2001 From: Bryce Kalow Date: Mon, 3 Jun 2024 16:56:07 -0500 Subject: [PATCH 2/6] Add tests for SignUpButton, remove aria-hidden on wrapping modal backdrop element --- .../next-app-router/src/app/buttons/page.tsx | 16 ++- integration/testUtils/commonPageObject.ts | 2 +- integration/tests/redirects.test.ts | 104 ++++++++++++++---- packages/clerk-js/src/ui/elements/Modal.tsx | 1 - 4 files changed, 100 insertions(+), 23 deletions(-) diff --git a/integration/templates/next-app-router/src/app/buttons/page.tsx b/integration/templates/next-app-router/src/app/buttons/page.tsx index 0755a45c48..4e4a500e72 100644 --- a/integration/templates/next-app-router/src/app/buttons/page.tsx +++ b/integration/templates/next-app-router/src/app/buttons/page.tsx @@ -1,4 +1,4 @@ -import { SignInButton } from '@clerk/nextjs'; +import { SignInButton, SignUpButton } from '@clerk/nextjs'; export default function Home() { return ( @@ -16,6 +16,20 @@ export default function Home() { > Sign in button (fallback) + + + Sign up button (force) + + + + Sign up button (fallback) + ); } diff --git a/integration/testUtils/commonPageObject.ts b/integration/testUtils/commonPageObject.ts index 352a143465..3243a35748 100644 --- a/integration/testUtils/commonPageObject.ts +++ b/integration/testUtils/commonPageObject.ts @@ -3,7 +3,7 @@ import type { TestArgs } from './signInPageObject'; export const common = ({ page }: TestArgs) => { const self = { continue: () => { - return page.locator('button').filter({ hasText: 'Continue' }).click(); + return page.getByRole('button', { name: 'Continue', exact: true }).click(); }, setPassword: (val: string) => { return page.locator('input[name=password]').fill(val); diff --git a/integration/tests/redirects.test.ts b/integration/tests/redirects.test.ts index 50ef366cb8..ce8d1434e0 100644 --- a/integration/tests/redirects.test.ts +++ b/integration/tests/redirects.test.ts @@ -30,37 +30,101 @@ testAgainstRunningApps({ withEnv: [appConfigs.envs.withEmailCodes] })('redirect await u.page.context().clearCookies(); }); - test('sign in button respects forceRedirectUrl', async ({ page, context }) => { - const u = createTestUtils({ app, page, context }); + test.describe('SignInButton', () => { + test('sign in button respects forceRedirectUrl', async ({ page, context }) => { + const u = createTestUtils({ app, page, context }); + + await u.page.goToRelative('/buttons'); + await u.page.waitForClerkJsLoaded(); + await u.po.expect.toBeSignedOut(); + + await u.page.getByText('Sign in button (force)').click(); - await u.page.goToRelative('/buttons'); - await u.page.waitForClerkJsLoaded(); - await u.po.expect.toBeSignedOut(); + await u.po.signIn.waitForMounted(); + await u.po.signIn.signInWithEmailAndInstantPassword({ email: fakeUser.email, password: fakeUser.password }); + + await u.page.waitForAppUrl('/protected'); + + await u.po.expect.toBeSignedIn(); + }); - await u.page.getByText('Sign in button (force)').click(); + test('sign in button respects fallbackRedirectUrl', async ({ page, context }) => { + const u = createTestUtils({ app, page, context }); - await u.po.signIn.waitForMounted(); - await u.po.signIn.signInWithEmailAndInstantPassword({ email: fakeUser.email, password: fakeUser.password }); + await u.page.goToRelative('/buttons'); + await u.page.waitForClerkJsLoaded(); + await u.po.expect.toBeSignedOut(); - await u.page.waitForAppUrl('/protected'); + await u.page.getByText('Sign in button (fallback)').click(); - await u.po.expect.toBeSignedIn(); + await u.po.signIn.waitForMounted(); + await u.po.signIn.signInWithEmailAndInstantPassword({ email: fakeUser.email, password: fakeUser.password }); + + await u.page.waitForAppUrl('/protected'); + + await u.po.expect.toBeSignedIn(); + }); }); - test('sign in button respects fallbackRedirectUrl', async ({ page, context }) => { - const u = createTestUtils({ app, page, context }); + test.describe('SignUpButton', () => { + test('sign up button respects forceRedirectUrl', async ({ page, context }) => { + const u = createTestUtils({ app, page, context }); + const fakeUser = u.services.users.createFakeUser({ + fictionalEmail: true, + withPhoneNumber: true, + withUsername: true, + }); + + await u.page.goToRelative('/buttons'); + await u.page.waitForClerkJsLoaded(); - await u.page.goToRelative('/buttons'); - await u.page.waitForClerkJsLoaded(); - await u.po.expect.toBeSignedOut(); + await u.page.getByText('Sign up button (force)').click(); - await u.page.getByText('Sign in button (fallback)').click(); + // Fill in sign up form + await u.po.signUp.signUpWithEmailAndPassword({ + email: fakeUser.email, + password: fakeUser.password, + }); - await u.po.signIn.waitForMounted(); - await u.po.signIn.signInWithEmailAndInstantPassword({ email: fakeUser.email, password: fakeUser.password }); + // Verify email + await u.po.signUp.enterTestOtpCode(); - await u.page.waitForAppUrl('/protected'); + await u.page.waitForAppUrl('/protected'); - await u.po.expect.toBeSignedIn(); + // Check if user is signed in + await u.po.expect.toBeSignedIn(); + + await fakeUser.deleteIfExists(); + }); + + test('sign up button respects fallbackRedirectUrl', async ({ page, context }) => { + const u = createTestUtils({ app, page, context }); + const fakeUser = u.services.users.createFakeUser({ + fictionalEmail: true, + withPhoneNumber: true, + withUsername: true, + }); + + await u.page.goToRelative('/buttons'); + await u.page.waitForClerkJsLoaded(); + + await u.page.getByText('Sign up button (fallback)').click(); + + // Fill in sign up form + await u.po.signUp.signUpWithEmailAndPassword({ + email: fakeUser.email, + password: fakeUser.password, + }); + + // Verify email + await u.po.signUp.enterTestOtpCode(); + + await u.page.waitForAppUrl('/protected'); + + // Check if user is signed in + await u.po.expect.toBeSignedIn(); + + await fakeUser.deleteIfExists(); + }); }); }); diff --git a/packages/clerk-js/src/ui/elements/Modal.tsx b/packages/clerk-js/src/ui/elements/Modal.tsx index 5815040fcc..b65a057e50 100644 --- a/packages/clerk-js/src/ui/elements/Modal.tsx +++ b/packages/clerk-js/src/ui/elements/Modal.tsx @@ -50,7 +50,6 @@ export const Modal = withFloatingTree((props: ModalProps) => { > Date: Mon, 3 Jun 2024 16:58:39 -0500 Subject: [PATCH 3/6] Remove commented out code --- packages/clerk-js/src/ui/elements/Modal.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/clerk-js/src/ui/elements/Modal.tsx b/packages/clerk-js/src/ui/elements/Modal.tsx index b65a057e50..a5cd6a734f 100644 --- a/packages/clerk-js/src/ui/elements/Modal.tsx +++ b/packages/clerk-js/src/ui/elements/Modal.tsx @@ -57,7 +57,6 @@ export const Modal = withFloatingTree((props: ModalProps) => { animation: `${animations.fadeIn} 150ms ${t.transitionTiming.$common}`, zIndex: t.zIndices.$modal, backgroundColor: t.colors.$modalBackdrop, - // ...common.centeredFlex(), alignItems: 'flex-start', justifyContent: 'center', overflow: 'auto', From 298ac2a2c4576a3b936271315592ea845f0f46f5 Mon Sep 17 00:00:00 2001 From: Bryce Kalow Date: Mon, 3 Jun 2024 17:03:30 -0500 Subject: [PATCH 4/6] Adds changesets --- .changeset/metal-foxes-raise.md | 5 +++++ .changeset/sixty-ears-rest.md | 5 +++++ 2 files changed, 10 insertions(+) create mode 100644 .changeset/metal-foxes-raise.md create mode 100644 .changeset/sixty-ears-rest.md diff --git a/.changeset/metal-foxes-raise.md b/.changeset/metal-foxes-raise.md new file mode 100644 index 0000000000..b960dae386 --- /dev/null +++ b/.changeset/metal-foxes-raise.md @@ -0,0 +1,5 @@ +--- +'@clerk/clerk-react': patch +--- + +Update `SignUpButton` and `SignInButton` to respect `forceRedirect` and `fallbackRedirect` props. Previously, these were getting ignored and successful completions of the flows would fallback to the default redirect URL. diff --git a/.changeset/sixty-ears-rest.md b/.changeset/sixty-ears-rest.md new file mode 100644 index 0000000000..c59bdb3a1e --- /dev/null +++ b/.changeset/sixty-ears-rest.md @@ -0,0 +1,5 @@ +--- +'@clerk/clerk-js': patch +--- + +Fixed a bug where Clerk components rendered in modals were wrapped with `aria-hidden`. From 20f94bad50c7f9c361113f6c70ce4396922099a5 Mon Sep 17 00:00:00 2001 From: Bryce Kalow Date: Mon, 3 Jun 2024 17:15:13 -0500 Subject: [PATCH 5/6] Ensure the namespaced props are passed to the redirectTo* methods --- packages/react/src/components/SignInButton.tsx | 6 +++++- packages/react/src/components/SignUpButton.tsx | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/react/src/components/SignInButton.tsx b/packages/react/src/components/SignInButton.tsx index 796f68220f..6dd8453a02 100644 --- a/packages/react/src/components/SignInButton.tsx +++ b/packages/react/src/components/SignInButton.tsx @@ -22,7 +22,11 @@ export const SignInButton = withClerk(({ clerk, children, ...props }: WithClerkP if (mode === 'modal') { return clerk.openSignIn(opts); } - return clerk.redirectToSignIn(opts); + return clerk.redirectToSignIn({ + ...opts, + signInFallbackRedirectUrl: fallbackRedirectUrl, + signInForceRedirectUrl: forceRedirectUrl, + }); }; const wrappedChildClickHandler: React.MouseEventHandler = async e => { diff --git a/packages/react/src/components/SignUpButton.tsx b/packages/react/src/components/SignUpButton.tsx index 8a212fe5f9..729494550a 100644 --- a/packages/react/src/components/SignUpButton.tsx +++ b/packages/react/src/components/SignUpButton.tsx @@ -32,7 +32,11 @@ export const SignUpButton = withClerk(({ clerk, children, ...props }: WithClerkP return clerk.openSignUp(opts); } - return clerk.redirectToSignUp(opts); + return clerk.redirectToSignUp({ + ...opts, + signUpFallbackRedirectUrl: fallbackRedirectUrl, + signUpForceRedirectUrl: forceRedirectUrl, + }); }; const wrappedChildClickHandler: React.MouseEventHandler = async e => { From 90d214c27f6f6dcfc66e70626f816e5e8394c23b Mon Sep 17 00:00:00 2001 From: Bryce Kalow Date: Mon, 3 Jun 2024 17:34:18 -0500 Subject: [PATCH 6/6] Fix unit tests --- packages/react/src/components/__tests__/SignInButton.test.tsx | 3 ++- packages/react/src/components/__tests__/SignUpButton.test.tsx | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/react/src/components/__tests__/SignInButton.test.tsx b/packages/react/src/components/__tests__/SignInButton.test.tsx index 22050952ef..a1b14241b9 100644 --- a/packages/react/src/components/__tests__/SignInButton.test.tsx +++ b/packages/react/src/components/__tests__/SignInButton.test.tsx @@ -52,7 +52,7 @@ describe('', () => { const btn = screen.getByText('Sign in'); await userEvent.click(btn); - expect(mockRedirectToSignIn).toHaveBeenCalledWith({ signInForceRedirectUrl: url }); + expect(mockRedirectToSignIn).toHaveBeenCalledWith({ forceRedirectUrl: url, signInForceRedirectUrl: url }); }); it('handles fallbackRedirectUrl prop', async () => { @@ -62,6 +62,7 @@ describe('', () => { await userEvent.click(btn); expect(mockRedirectToSignIn).toHaveBeenCalledWith({ + fallbackRedirectUrl: url, signInFallbackRedirectUrl: url, }); }); diff --git a/packages/react/src/components/__tests__/SignUpButton.test.tsx b/packages/react/src/components/__tests__/SignUpButton.test.tsx index 918ecf8baa..b1f9f4e755 100644 --- a/packages/react/src/components/__tests__/SignUpButton.test.tsx +++ b/packages/react/src/components/__tests__/SignUpButton.test.tsx @@ -53,7 +53,7 @@ describe('', () => { const btn = screen.getByText('Sign up'); userEvent.click(btn); await waitFor(() => { - expect(mockRedirectToSignUp).toHaveBeenCalledWith({ signUpForceRedirectUrl: url }); + expect(mockRedirectToSignUp).toHaveBeenCalledWith({ forceRedirectUrl: url, signUpForceRedirectUrl: url }); }); }); @@ -63,6 +63,7 @@ describe('', () => { userEvent.click(btn); await waitFor(() => { expect(mockRedirectToSignUp).toHaveBeenCalledWith({ + fallbackRedirectUrl: url, signUpFallbackRedirectUrl: url, }); });