From 3c6be04f624c1c51942d643edca33d00f171190c Mon Sep 17 00:00:00 2001 From: panteliselef Date: Tue, 2 Jan 2024 10:53:23 +0200 Subject: [PATCH 1/2] fix(clerk-js): Avoid triggering prepare verification twice Use `useFetch` to cache the ongoing request and not allow another to be fired until the first one has resolved. --- .changeset/flat-bugs-visit.md | 5 ++++ .../components/SignUp/SignUpEmailCodeCard.tsx | 23 ++++++++++++------- .../components/SignUp/SignUpPhoneCodeCard.tsx | 23 ++++++++++++------- packages/clerk-js/src/ui/hooks/useFetch.ts | 11 +++++---- 4 files changed, 42 insertions(+), 20 deletions(-) create mode 100644 .changeset/flat-bugs-visit.md diff --git a/.changeset/flat-bugs-visit.md b/.changeset/flat-bugs-visit.md new file mode 100644 index 00000000000..eda2e695baa --- /dev/null +++ b/.changeset/flat-bugs-visit.md @@ -0,0 +1,5 @@ +--- +'@clerk/clerk-js': patch +--- + +Bug fix: Avoid triggering prepare verification twice. (Affects only dev mode) diff --git a/packages/clerk-js/src/ui/components/SignUp/SignUpEmailCodeCard.tsx b/packages/clerk-js/src/ui/components/SignUp/SignUpEmailCodeCard.tsx index b52226d286b..4f143706316 100644 --- a/packages/clerk-js/src/ui/components/SignUp/SignUpEmailCodeCard.tsx +++ b/packages/clerk-js/src/ui/components/SignUp/SignUpEmailCodeCard.tsx @@ -1,18 +1,11 @@ -import React from 'react'; - import { useCoreSignUp } from '../../contexts'; import { Flow, localizationKeys } from '../../customizables'; +import { useFetch } from '../../hooks'; import { SignUpVerificationCodeForm } from './SignUpVerificationCodeForm'; export const SignUpEmailCodeCard = () => { const signUp = useCoreSignUp(); - React.useEffect(() => { - // TODO: This prepare method is not idempotent. - // We need to make sure that R18 won't trigger this twice - void prepare(); - }, []); - const prepare = () => { const emailVerificationStatus = signUp.verifications.emailAddress.status; if (!signUp.status || emailVerificationStatus === 'verified') { @@ -21,6 +14,20 @@ export const SignUpEmailCodeCard = () => { return signUp.prepareEmailAddressVerification({ strategy: 'email_code' }); }; + // TODO: Introduce a useMutation to handle mutating requests + useFetch( + // @ts-ignore Typescript complains because prepare may return undefined + prepare, + { + name: 'prepare', + strategy: 'email_code', + number: signUp.emailAddress, + }, + { + staleTime: 100, + }, + ); + const attempt = (code: string) => signUp.attemptEmailAddressVerification({ code }); return ( diff --git a/packages/clerk-js/src/ui/components/SignUp/SignUpPhoneCodeCard.tsx b/packages/clerk-js/src/ui/components/SignUp/SignUpPhoneCodeCard.tsx index 46407f4bee8..f40835b23c7 100644 --- a/packages/clerk-js/src/ui/components/SignUp/SignUpPhoneCodeCard.tsx +++ b/packages/clerk-js/src/ui/components/SignUp/SignUpPhoneCodeCard.tsx @@ -1,19 +1,12 @@ -import React from 'react'; - import { useCoreSignUp } from '../../contexts'; import { Flow, localizationKeys } from '../../customizables'; import { withCardStateProvider } from '../../elements'; +import { useFetch } from '../../hooks'; import { SignUpVerificationCodeForm } from './SignUpVerificationCodeForm'; export const SignUpPhoneCodeCard = withCardStateProvider(() => { const signUp = useCoreSignUp(); - React.useEffect(() => { - // TODO: This prepare method is not idempotent. - // We need to make sure that R18 won't trigger this twice - void prepare(); - }, []); - const prepare = () => { const phoneVerificationStatus = signUp.verifications.phoneNumber.status; if (!signUp.status || phoneVerificationStatus === 'verified') { @@ -22,6 +15,20 @@ export const SignUpPhoneCodeCard = withCardStateProvider(() => { return signUp.preparePhoneNumberVerification({ strategy: 'phone_code' }); }; + // TODO: Introduce a useMutation to handle mutating requests + useFetch( + // @ts-ignore Typescript complains because prepare may return undefined + prepare, + { + name: 'prepare', + strategy: 'phone_code', + number: signUp.phoneNumber, + }, + { + staleTime: 100, + }, + ); + const attempt = (code: string) => signUp.attemptPhoneNumberVerification({ code }); return ( diff --git a/packages/clerk-js/src/ui/hooks/useFetch.ts b/packages/clerk-js/src/ui/hooks/useFetch.ts index 99e3b23028b..8e717b5aec9 100644 --- a/packages/clerk-js/src/ui/hooks/useFetch.ts +++ b/packages/clerk-js/src/ui/hooks/useFetch.ts @@ -37,7 +37,7 @@ const useCache = ( key: K, ): { getCache: () => State | undefined; - setCache: (state: State) => void; + setCache: (state: State) => void; subscribeCache: (callback: () => void) => () => void; } => { const serializedKey = serialize(key); @@ -64,18 +64,21 @@ const useCache = ( export const useFetch = ( fetcher: ((...args: any) => Promise) | undefined, params: K, - callbacks?: { + options?: { onSuccess?: (data: T) => void; + staleTime?: number; }, ) => { const { subscribeCache, getCache, setCache } = useCache(params); + + const staleTime = options?.staleTime || 1000 * 60 * 2; //cache for 2 minutes by default const fetcherRef = useRef(fetcher); const cached = useSyncExternalStore(subscribeCache, getCache); useEffect(() => { const fetcherMissing = !fetcherRef.current; - const isCacheStale = Date.now() - (getCache()?.cachedAt || 0) < 1000 * 60 * 2; //cache for 2 minutes; + const isCacheStale = Date.now() - (getCache()?.cachedAt || 0) < staleTime; const isRequestOnGoing = getCache()?.isValidating; if (fetcherMissing || isCacheStale || isRequestOnGoing) { @@ -99,7 +102,7 @@ export const useFetch = ( error: null, cachedAt: Date.now(), }); - callbacks?.onSuccess?.(data); + options?.onSuccess?.(data); } }) .catch(() => { From e4c600d2aec0d3e6304e309969d337c21c4320b0 Mon Sep 17 00:00:00 2001 From: panteliselef Date: Tue, 2 Jan 2024 18:14:21 +0200 Subject: [PATCH 2/2] chore(clerk-js): Fix tests --- .../components/SignUp/SignUpEmailCodeCard.tsx | 9 ++++---- .../components/SignUp/SignUpPhoneCodeCard.tsx | 10 ++++---- .../__tests__/SignUpVerifyEmail.test.tsx | 23 +++++++------------ .../__tests__/SignUpVerifyPhone.test.tsx | 10 +++++--- 4 files changed, 25 insertions(+), 27 deletions(-) diff --git a/packages/clerk-js/src/ui/components/SignUp/SignUpEmailCodeCard.tsx b/packages/clerk-js/src/ui/components/SignUp/SignUpEmailCodeCard.tsx index 4f143706316..355e76b1d3d 100644 --- a/packages/clerk-js/src/ui/components/SignUp/SignUpEmailCodeCard.tsx +++ b/packages/clerk-js/src/ui/components/SignUp/SignUpEmailCodeCard.tsx @@ -6,9 +6,11 @@ import { SignUpVerificationCodeForm } from './SignUpVerificationCodeForm'; export const SignUpEmailCodeCard = () => { const signUp = useCoreSignUp(); + const emailVerificationStatus = signUp.verifications.emailAddress.status; + const shouldAvoidPrepare = !signUp.status || emailVerificationStatus === 'verified'; + const prepare = () => { - const emailVerificationStatus = signUp.verifications.emailAddress.status; - if (!signUp.status || emailVerificationStatus === 'verified') { + if (shouldAvoidPrepare) { return; } return signUp.prepareEmailAddressVerification({ strategy: 'email_code' }); @@ -16,8 +18,7 @@ export const SignUpEmailCodeCard = () => { // TODO: Introduce a useMutation to handle mutating requests useFetch( - // @ts-ignore Typescript complains because prepare may return undefined - prepare, + shouldAvoidPrepare ? undefined : () => signUp.prepareEmailAddressVerification({ strategy: 'email_code' }), { name: 'prepare', strategy: 'email_code', diff --git a/packages/clerk-js/src/ui/components/SignUp/SignUpPhoneCodeCard.tsx b/packages/clerk-js/src/ui/components/SignUp/SignUpPhoneCodeCard.tsx index f40835b23c7..8cbaf3ee9b2 100644 --- a/packages/clerk-js/src/ui/components/SignUp/SignUpPhoneCodeCard.tsx +++ b/packages/clerk-js/src/ui/components/SignUp/SignUpPhoneCodeCard.tsx @@ -7,9 +7,10 @@ import { SignUpVerificationCodeForm } from './SignUpVerificationCodeForm'; export const SignUpPhoneCodeCard = withCardStateProvider(() => { const signUp = useCoreSignUp(); + const phoneVerificationStatus = signUp.verifications.phoneNumber.status; + const shouldAvoidPrepare = !signUp.status || phoneVerificationStatus === 'verified'; const prepare = () => { - const phoneVerificationStatus = signUp.verifications.phoneNumber.status; - if (!signUp.status || phoneVerificationStatus === 'verified') { + if (shouldAvoidPrepare) { return; } return signUp.preparePhoneNumberVerification({ strategy: 'phone_code' }); @@ -17,10 +18,9 @@ export const SignUpPhoneCodeCard = withCardStateProvider(() => { // TODO: Introduce a useMutation to handle mutating requests useFetch( - // @ts-ignore Typescript complains because prepare may return undefined - prepare, + shouldAvoidPrepare ? undefined : () => signUp.preparePhoneNumberVerification({ strategy: 'phone_code' }), { - name: 'prepare', + name: 'signUp.preparePhoneNumberVerification', strategy: 'phone_code', number: signUp.phoneNumber, }, diff --git a/packages/clerk-js/src/ui/components/SignUp/__tests__/SignUpVerifyEmail.test.tsx b/packages/clerk-js/src/ui/components/SignUp/__tests__/SignUpVerifyEmail.test.tsx index 9cd0189dc5c..3a17f562c60 100644 --- a/packages/clerk-js/src/ui/components/SignUp/__tests__/SignUpVerifyEmail.test.tsx +++ b/packages/clerk-js/src/ui/components/SignUp/__tests__/SignUpVerifyEmail.test.tsx @@ -14,10 +14,11 @@ describe('SignUpVerifyEmail', () => { }); it('shows the email associated with the sign up', async () => { - const { wrapper } = await createFixtures(f => { + const { wrapper, fixtures } = await createFixtures(f => { f.withEmailAddress({ required: true }); f.startSignUpWithEmailAddress({ emailAddress: 'test@clerk.com' }); }); + fixtures.signUp.prepareEmailAddressVerification.mockRejectedValue(null); render(, { wrapper }); screen.getByText('test@clerk.com'); }); @@ -44,13 +45,8 @@ describe('SignUpVerifyEmail', () => { f.withEmailAddress({ required: true, verifications: ['email_code'] }); f.startSignUpWithEmailAddress({ emailAddress: 'test@clerk.com' }); }); - fixtures.signUp.createEmailLinkFlow.mockImplementation( - () => - ({ - startEmailLinkFlow: jest.fn(() => new Promise(() => ({}))), - cancelEmailLinkFlow: jest.fn(() => new Promise(() => ({}))), - } as any), - ); + + fixtures.signUp.prepareEmailAddressVerification.mockRejectedValue(null); render(, { wrapper }); screen.getByText(/Verify your email/i); @@ -62,6 +58,8 @@ describe('SignUpVerifyEmail', () => { f.withEmailAddress({ required: true }); f.startSignUpWithEmailAddress({ emailAddress: 'test@clerk.com' }); }); + fixtures.signUp.prepareEmailAddressVerification.mockRejectedValue(null); + const { userEvent } = render(, { wrapper }); await userEvent.click( screen.getByRole('button', { @@ -94,13 +92,8 @@ describe('SignUpVerifyEmail', () => { f.withEmailAddress({ required: true, verifications: ['email_code'] }); f.startSignUpWithEmailAddress({ emailAddress: 'test@clerk.com' }); }); - fixtures.signUp.createEmailLinkFlow.mockImplementation( - () => - ({ - startEmailLinkFlow: jest.fn(() => new Promise(() => ({}))), - cancelEmailLinkFlow: jest.fn(() => new Promise(() => ({}))), - } as any), - ); + + fixtures.signUp.prepareEmailAddressVerification.mockRejectedValue(null); render(, { wrapper }); const resendButton = screen.getByText(/Resend/i); diff --git a/packages/clerk-js/src/ui/components/SignUp/__tests__/SignUpVerifyPhone.test.tsx b/packages/clerk-js/src/ui/components/SignUp/__tests__/SignUpVerifyPhone.test.tsx index bf5d6ddfe7e..2d6b440dfa9 100644 --- a/packages/clerk-js/src/ui/components/SignUp/__tests__/SignUpVerifyPhone.test.tsx +++ b/packages/clerk-js/src/ui/components/SignUp/__tests__/SignUpVerifyPhone.test.tsx @@ -14,19 +14,21 @@ describe('SignUpVerifyPhone', () => { }); it('shows the phone number associated with the sign up', async () => { - const { wrapper } = await createFixtures(f => { + const { wrapper, fixtures } = await createFixtures(f => { f.withPhoneNumber({ required: true }); f.startSignUpWithPhoneNumber({ phoneNumber: '+306911111111' }); }); + fixtures.signUp.preparePhoneNumberVerification.mockRejectedValue(null); render(, { wrapper }); screen.getByText('+30 691 1111111'); }); it('shows the verify with code message', async () => { - const { wrapper } = await createFixtures(f => { + const { wrapper, fixtures } = await createFixtures(f => { f.withPhoneNumber({ required: true }); f.startSignUpWithPhoneNumber(); }); + fixtures.signUp.preparePhoneNumberVerification.mockRejectedValue(null); render(, { wrapper }); screen.getByText(/Verify your phone/i); screen.getByText(/Enter the verification code sent to your phone/i); @@ -37,6 +39,7 @@ describe('SignUpVerifyPhone', () => { f.withPhoneNumber({ required: true }); f.startSignUpWithPhoneNumber(); }); + fixtures.signUp.preparePhoneNumberVerification.mockRejectedValue(null); const { userEvent } = render(, { wrapper }); await userEvent.click( screen.getByRole('button', { @@ -47,10 +50,11 @@ describe('SignUpVerifyPhone', () => { }); it('Resend code button exists', async () => { - const { wrapper } = await createFixtures(f => { + const { wrapper, fixtures } = await createFixtures(f => { f.withPhoneNumber({ required: true }); f.startSignUpWithEmailAddress({ emailAddress: 'test@clerk.com' }); }); + fixtures.signUp.preparePhoneNumberVerification.mockRejectedValue(null); render(, { wrapper }); const resendButton = screen.getByText(/Resend/i); expect(resendButton.tagName.toUpperCase()).toBe('BUTTON');