-
Notifications
You must be signed in to change notification settings - Fork 283
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-react): Fix forceRedirectUrl and fallbackRedirectUrl when passed to button components #3508
fix(clerk-react): Fix forceRedirectUrl and fallbackRedirectUrl when passed to button components #3508
Changes from 5 commits
2c0b901
2a293af
133f5c7
298ac2a
20f94ba
90d214c
ae5ac6d
df2f087
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,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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@clerk/clerk-js': patch | ||
--- | ||
|
||
Fixed a bug where Clerk components rendered in modals were wrapped with `aria-hidden`. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
import { SignInButton, SignUpButton } from '@clerk/nextjs'; | ||
|
||
export default function Home() { | ||
return ( | ||
<main> | ||
<SignInButton | ||
mode='modal' | ||
forceRedirectUrl='/protected' | ||
> | ||
Sign in button (force) | ||
</SignInButton> | ||
|
||
<SignInButton | ||
mode='modal' | ||
fallbackRedirectUrl='/protected' | ||
> | ||
Sign in button (fallback) | ||
</SignInButton> | ||
|
||
<SignUpButton | ||
mode='modal' | ||
forceRedirectUrl='/protected' | ||
> | ||
Sign up button (force) | ||
</SignUpButton> | ||
|
||
<SignUpButton | ||
mode='modal' | ||
fallbackRedirectUrl='/protected' | ||
> | ||
Sign up button (fallback) | ||
</SignUpButton> | ||
</main> | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,130 @@ | ||
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.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.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(); | ||
}); | ||
}); | ||
|
||
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.getByText('Sign up button (force)').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(); | ||
}); | ||
|
||
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(); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
import type { SignInProps } from '@clerk/types'; | ||
import React from 'react'; | ||
|
||
import type { SignInButtonProps, WithClerkProp } from '../types'; | ||
|
@@ -11,21 +12,27 @@ export const SignInButton = withClerk(({ clerk, children, ...props }: WithClerkP | |
const child = assertSingleChild(children)('SignInButton'); | ||
|
||
const clickHandler = () => { | ||
const opts = { | ||
const opts: SignInProps = { | ||
forceRedirectUrl, | ||
fallbackRedirectUrl, | ||
Comment on lines
+15
to
+17
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. The modal mode relies on the non-prefixed props, adding the type here revealed the issue. |
||
signUpFallbackRedirectUrl, | ||
signUpForceRedirectUrl, | ||
signInForceRedirectUrl: forceRedirectUrl, | ||
signInFallbackRedirectUrl: fallbackRedirectUrl, | ||
}; | ||
|
||
if (mode === 'modal') { | ||
return clerk.openSignIn(opts); | ||
} | ||
return clerk.redirectToSignIn(opts); | ||
return clerk.redirectToSignIn({ | ||
...opts, | ||
signInFallbackRedirectUrl: fallbackRedirectUrl, | ||
signInForceRedirectUrl: forceRedirectUrl, | ||
Comment on lines
+27
to
+28
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. when calling the redirect methods, the prefixed props are necessary. Added here. |
||
}); | ||
}; | ||
|
||
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); | ||
} | ||
Comment on lines
+33
to
+35
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. Some auto-fix of a lint rule caused a type error, so I tweaked the logic a bit here as a result. 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. Interesting. |
||
return clickHandler(); | ||
}; | ||
|
||
|
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 don't think we want this, as it removes the entire modal from the accessibility tree.
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.
Definitely not 😅