From 76533dcd2de3a1e229f088d23489ddcfbb20b818 Mon Sep 17 00:00:00 2001 From: Maxim Khvatalin Date: Thu, 14 May 2020 14:45:54 +0300 Subject: [PATCH] Fix Elements initialization in React Strict/Concurrent mode Refs cannot be mutated and used to update state in the same time in rendering phase. As this is side-effect, it can produce various bugs in concurrent mode. In StrictMode Elements doesn't do transition from null to valid stripe instance. As in StrictMode React renders twice, `final` ref becomes `true`, but `ctx` state isn't changed. References: https://reactjs.org/docs/strict-mode.html#detecting-unexpected-side-effects https://github.com/facebook/react/issues/18003 https://github.com/facebook/react/pull/18545 --- src/components/Elements.test.tsx | 28 ++++++- src/components/Elements.tsx | 108 +++++++++----------------- src/utils/usePrevious.test.tsx | 30 ------- src/utils/usePrevious.ts | 11 --- src/utils/usePromiseResolver.test.tsx | 60 ++++++++++++++ src/utils/usePromiseResolver.ts | 51 ++++++++++++ 6 files changed, 172 insertions(+), 116 deletions(-) delete mode 100644 src/utils/usePrevious.test.tsx delete mode 100644 src/utils/usePrevious.ts create mode 100644 src/utils/usePromiseResolver.test.tsx create mode 100644 src/utils/usePromiseResolver.ts diff --git a/src/components/Elements.test.tsx b/src/components/Elements.test.tsx index f8a6be2..3445a65 100644 --- a/src/components/Elements.test.tsx +++ b/src/components/Elements.test.tsx @@ -152,6 +152,30 @@ describe('Elements', () => { expect(result.current).toBe(mockElements); }); + test('allows a transition from null to a valid Stripe object in StrictMode', async () => { + let stripeProp: any = null; + const spy = jest.fn(); + const TestComponent = () => ( + + + + {({stripe, elements}) => { + spy({stripe, elements}); + return null; + }} + + + + ); + + const {rerender} = render(); + expect(spy).toBeCalledWith({stripe: null, elements: null}); + + stripeProp = mockStripe; + rerender(); + expect(spy).toBeCalledWith({stripe: mockStripe, elements: mockElements}); + }); + test('works with a Promise resolving to a valid Stripe object', async () => { const wrapper = ({children}: any) => ( {children} @@ -225,9 +249,7 @@ describe('Elements', () => { expect(mockStripe.elements).toHaveBeenCalledWith({foo: 'foo'}); }); - // TODO(christopher): support Strict Mode first - // eslint-disable-next-line jest/no-disabled-tests - test.skip('does not allow updates to options after the Stripe Promise is set in StrictMode', async () => { + test('does not allow updates to options after the Stripe Promise is set in StrictMode', async () => { // Silence console output so test output is less noisy consoleWarn.mockImplementation(() => {}); diff --git a/src/components/Elements.tsx b/src/components/Elements.tsx index ad6c004..3552e96 100644 --- a/src/components/Elements.tsx +++ b/src/components/Elements.tsx @@ -6,8 +6,8 @@ import React from 'react'; import PropTypes from 'prop-types'; import {isEqual} from '../utils/isEqual'; -import {usePrevious} from '../utils/usePrevious'; -import {isStripe, isPromise} from '../utils/guards'; +import {usePromiseResolver} from '../utils/usePromiseResolver'; +import {isStripe} from '../utils/guards'; const INVALID_STRIPE_ERROR = 'Invalid prop `stripe` supplied to `Elements`. We recommend using the `loadStripe` utility from `@stripe/stripe-js`. See https://stripe.com/docs/stripe-js/react#elements-props-stripe for details.'; @@ -23,28 +23,6 @@ const validateStripe = (maybeStripe: unknown): null | stripeJs.Stripe => { throw new Error(INVALID_STRIPE_ERROR); }; -type ParsedStripeProp = - | {tag: 'empty'} - | {tag: 'sync'; stripe: stripeJs.Stripe} - | {tag: 'async'; stripePromise: Promise}; - -const parseStripeProp = (raw: unknown): ParsedStripeProp => { - if (isPromise(raw)) { - return { - tag: 'async', - stripePromise: Promise.resolve(raw).then(validateStripe), - }; - } - - const stripe = validateStripe(raw); - - if (stripe === null) { - return {tag: 'empty'}; - } - - return {tag: 'sync', stripe}; -}; - interface ElementsContextValue { elements: stripeJs.StripeElements | null; stripe: stripeJs.Stripe | null; @@ -66,6 +44,14 @@ export const parseElementsContext = ( return ctx; }; +const createElementsContext = (stripe: stripeJs.Stripe | null, options?: stripeJs.StripeElementsOptions) => { + const elements = stripe ? stripe.elements(options) : null + return { + stripe, + elements + } +} + interface ElementsProps { /** * A [Stripe object](https://stripe.com/docs/js/initializing) or a `Promise` resolving to a `Stripe` object. @@ -99,66 +85,44 @@ interface PrivateElementsProps { * * @docs https://stripe.com/docs/stripe-js/react#elements-provider */ -export const Elements: FunctionComponent = ({ - stripe: rawStripeProp, - options, - children, -}: PrivateElementsProps) => { - const final = React.useRef(false); - const isMounted = React.useRef(true); - const parsed = React.useMemo(() => parseStripeProp(rawStripeProp), [ - rawStripeProp, - ]); - const [ctx, setContext] = React.useState(() => ({ - stripe: null, - elements: null, - })); - - const prevStripe = usePrevious(rawStripeProp); - const prevOptions = usePrevious(options); - if (prevStripe !== null) { - if (prevStripe !== rawStripeProp) { +export const Elements: FunctionComponent = (props: PrivateElementsProps) => { + const { children } = props + + if (props.stripe === undefined) throw new Error(INVALID_STRIPE_ERROR); + + const [inputs, setInputs] = React.useState({ rawStripe: props.stripe, options: props.options }) + React.useEffect(() => { + const { rawStripe, options } = inputs + const { stripe: nextRawStripe, options: nextOptions } = props + + const canUpdate = rawStripe === null + const hasRawStripeChanged = rawStripe !== nextRawStripe + const hasOptionsChanged = !isEqual(options, nextOptions) + + if (hasRawStripeChanged && !canUpdate) { console.warn( 'Unsupported prop change on Elements: You cannot change the `stripe` prop after setting it.' ); } - if (!isEqual(options, prevOptions)) { + + if (hasOptionsChanged && !canUpdate) { console.warn( 'Unsupported prop change on Elements: You cannot change the `options` prop after setting the `stripe` prop.' ); } - } - if (!final.current) { - if (parsed.tag === 'sync') { - final.current = true; - setContext({ - stripe: parsed.stripe, - elements: parsed.stripe.elements(options), - }); - } + const nextInputs = { rawStripe: nextRawStripe, options: nextOptions } + if (hasRawStripeChanged && canUpdate) setInputs(nextInputs) + }, [inputs, props]) - if (parsed.tag === 'async') { - final.current = true; - parsed.stripePromise.then((stripe) => { - if (stripe && isMounted.current) { - // Only update Elements context if the component is still mounted - // and stripe is not null. We allow stripe to be null to make - // handling SSR easier. - setContext({ - stripe, - elements: stripe.elements(options), - }); - } - }); - } - } + const [maybeStripe = null] = usePromiseResolver(inputs.rawStripe) + const resolvedStripe = validateStripe(maybeStripe) + const [ctx, setContext] = React.useState(() => createElementsContext(resolvedStripe, inputs.options)); + const shouldInitialize = resolvedStripe !== null && ctx.stripe === null React.useEffect(() => { - return (): void => { - isMounted.current = false; - }; - }, []); + if (shouldInitialize) setContext(createElementsContext(resolvedStripe, inputs.options)) + }, [shouldInitialize, resolvedStripe, inputs.options]) React.useEffect(() => { const anyStripe: any = ctx.stripe; diff --git a/src/utils/usePrevious.test.tsx b/src/utils/usePrevious.test.tsx deleted file mode 100644 index 0440b42..0000000 --- a/src/utils/usePrevious.test.tsx +++ /dev/null @@ -1,30 +0,0 @@ -import {renderHook} from '@testing-library/react-hooks'; - -import {usePrevious} from './usePrevious'; - -describe('usePrevious', () => { - it('returns the initial value if it has not yet been changed', () => { - const {result} = renderHook(() => usePrevious('foo')); - - expect(result.current).toEqual('foo'); - }); - - it('returns the previous value after the it has been changed', () => { - let val = 'foo'; - const {result, rerender} = renderHook(() => usePrevious(val)); - - expect(result.current).toEqual('foo'); - - val = 'bar'; - rerender(); - expect(result.current).toEqual('foo'); - - val = 'baz'; - rerender(); - expect(result.current).toEqual('bar'); - - val = 'buz'; - rerender(); - expect(result.current).toEqual('baz'); - }); -}); diff --git a/src/utils/usePrevious.ts b/src/utils/usePrevious.ts deleted file mode 100644 index d278086..0000000 --- a/src/utils/usePrevious.ts +++ /dev/null @@ -1,11 +0,0 @@ -import React from 'react'; - -export const usePrevious = (value: T): T => { - const ref = React.useRef(value); - - React.useEffect(() => { - ref.current = value; - }, [value]); - - return ref.current; -}; diff --git a/src/utils/usePromiseResolver.test.tsx b/src/utils/usePromiseResolver.test.tsx new file mode 100644 index 0000000..bbe3dc5 --- /dev/null +++ b/src/utils/usePromiseResolver.test.tsx @@ -0,0 +1,60 @@ +import {renderHook, act} from '@testing-library/react-hooks'; +import {usePromiseResolver} from './usePromiseResolver'; +import { mockStripe } from '../../test/mocks'; + +const createImperativePromise = (): [Promise, (value?: unknown) => Promise, (reason?: any) => Promise] => { + let resolveFn: (value?: unknown) => Promise = () => Promise.resolve() + let rejectFn: (reason?: any) => Promise = () => Promise.resolve() + + const promise = new Promise((resolve, reject) => { + const createVoidPromise = () => promise.then(() => undefined, () => undefined) + + resolveFn = (value) => { + resolve(value) + return createVoidPromise() + } + + rejectFn = (reason) => { + reject(reason) + return createVoidPromise() + } + }) + + return [promise, resolveFn, rejectFn] +} + +describe('usePromiseResolver', () => { + let stripe: ReturnType; + + beforeEach(() => { + stripe = mockStripe(); + }) + + it('returns resolved on mount when not promise given', () => { + const {result} = renderHook(() => usePromiseResolver(stripe)); + expect(result.current).toEqual([stripe, undefined, 'resolved']) + }); + + it('returns pending on mount when promise given', () => { + const [promise] = createImperativePromise() + const {result} = renderHook(() => usePromiseResolver(promise)); + expect(result.current).toEqual([undefined, undefined, 'pending']) + }); + + it('returns resolved when given promise resolved', async () => { + const [promise, resolve] = createImperativePromise() + const {result} = renderHook(() => usePromiseResolver(promise)); + + await act(() => resolve(stripe)) + expect(result.current).toEqual([stripe, undefined, 'resolved']) + }); + + it('returns rejected when given promise rejected', async () => { + const [promise,, reject] = createImperativePromise() + const {result} = renderHook(() => usePromiseResolver(promise)); + + const error = new Error('Something went wrong') + await act(() => reject(error)) + expect(result.current).toEqual([undefined, error, 'rejected']) + }); +}); diff --git a/src/utils/usePromiseResolver.ts b/src/utils/usePromiseResolver.ts new file mode 100644 index 0000000..c9019ff --- /dev/null +++ b/src/utils/usePromiseResolver.ts @@ -0,0 +1,51 @@ +import React from 'react'; +import {isPromise} from '../utils/guards'; + +type PromisePending = [undefined, undefined, 'pending']; +type PromiseResolved = [T, undefined, 'resolved']; +type PromiseRejected = [undefined, any, 'rejected']; +type PromiseState = PromisePending | PromiseResolved | PromiseRejected; + +const createPending = (): PromisePending => [undefined, undefined, 'pending']; + +const createResolved = (value: T): PromiseResolved => [ + value, + undefined, + 'resolved', +]; + +const createRejected = (reason: any): PromiseRejected => [ + undefined, + reason, + 'rejected', +]; + +export const usePromiseResolver = ( + mayBePromise: T | PromiseLike +): PromiseState => { + const [state, setState] = React.useState>(() => + isPromise(mayBePromise) ? createPending() : createResolved(mayBePromise) + ); + + React.useEffect(() => { + if (!isPromise(mayBePromise)) return setState(createResolved(mayBePromise)); + + let isMounted = true; + + setState(createPending()); + mayBePromise + .then( + (resolved) => createResolved(resolved), + (error) => createRejected(error) + ) + .then((nextState) => { + if (isMounted) setState(nextState); + }); + + return () => { + isMounted = false; + }; + }, [mayBePromise]); + + return state; +};