From 5f6865b73f8c3ce7a4bc8338a411a48c6910e144 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 63d5ba5ad5a..86c3aeb8910 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 02a86d4c741d506dce2e583cdf02571df51529ba 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 86c3aeb8910..0c8bdaf5746 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 6c74f029ae3..561037a1f26 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 @@ -13,10 +13,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.dev' }); }); + fixtures.signUp.prepareEmailAddressVerification.mockRejectedValue(null); render(, { wrapper }); screen.getByText('test@clerk.dev'); }); @@ -43,13 +44,8 @@ describe('SignUpVerifyEmail', () => { f.withEmailAddress({ required: true, verifications: ['email_code'] }); f.startSignUpWithEmailAddress({ emailAddress: 'test@clerk.dev' }); }); - fixtures.signUp.createEmailLinkFlow.mockImplementation( - () => - ({ - startEmailLinkFlow: jest.fn(() => new Promise(() => ({}))), - cancelEmailLinkFlow: jest.fn(() => new Promise(() => ({}))), - } as any), - ); + + fixtures.signUp.prepareEmailAddressVerification.mockRejectedValue(null); render(, { wrapper }); screen.getAllByText(/Verification Code/i); @@ -60,6 +56,8 @@ describe('SignUpVerifyEmail', () => { f.withEmailAddress({ required: true }); f.startSignUpWithEmailAddress({ emailAddress: 'test@clerk.dev' }); }); + fixtures.signUp.prepareEmailAddressVerification.mockRejectedValue(null); + const { userEvent } = render(, { wrapper }); await userEvent.click( screen.getByRole('button', { @@ -92,13 +90,8 @@ describe('SignUpVerifyEmail', () => { f.withEmailAddress({ required: true, verifications: ['email_code'] }); f.startSignUpWithEmailAddress({ emailAddress: 'test@clerk.dev' }); }); - 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 78e0a17c5f0..acd3ef90a0b 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 @@ -13,19 +13,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.getAllByText(/Verification Code/i); }); @@ -35,6 +37,7 @@ describe('SignUpVerifyPhone', () => { f.withPhoneNumber({ required: true }); f.startSignUpWithPhoneNumber(); }); + fixtures.signUp.preparePhoneNumberVerification.mockRejectedValue(null); const { userEvent } = render(, { wrapper }); await userEvent.click( screen.getByRole('button', { @@ -45,10 +48,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.dev' }); }); + fixtures.signUp.preparePhoneNumberVerification.mockRejectedValue(null); render(, { wrapper }); const resendButton = screen.getByText(/Resend/i); expect(resendButton.tagName.toUpperCase()).toBe('BUTTON');