Fix Elements initialization in React Strict/Concurrent mode #93

khmm12 · 2020-05-14T13:06:54Z

Summary & motivation

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 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
facebook/react#18003
facebook/react#18545

Testing & documentation

Tests are passed and now it works in a test project with StrictMode.

khmm12 · 2020-05-14T13:07:48Z

src/components/Elements.tsx

-  }
+  const maybeStripe = usePromiseResolver(rawStripe)
+  const stripe = validateStripe(maybeStripe)
+  const [ctx, setContext] = React.useState<ElementsContextValue>(() => createElementsContext(null));


Now it's also possible to initialize context value on mount

khmm12 · 2020-05-14T13:10:54Z

src/components/Elements.tsx

-  const prevOptions = usePrevious(options);
-  if (prevStripe !== null) {
-    if (prevStripe !== rawStripeProp) {
+  const [inputs, setInputs] = React.useState({ rawStripe: rawStripeProp, options: optionsProp })


It should be more cleaner as we have state which we can trust (never changed after transition from null value).

christopher-stripe

Hey @khmm12! Thanks for this. Having React Stripe.js work with Strict Mode would be great.

I left some feedback on the PR. My comments are either marked with "blocking", which marks feedback I think we must address before merging, or with "nit", which marks minor stylistic suggestions that you should feel free to ignore if you disagree with.

In addition to my inline comments, I have one more blocking comment: given that the general idea of these changes is to make React Stripe.js work with Strict Mode, could you add a test to that effect? For example, showing that a basic React Stripe.js integration doesn't error if it's wrapped with <React.StrictMode>.

christopher-stripe · 2020-05-20T17:00:11Z

src/components/Elements.tsx

-  if (prevStripe !== null) {
-    if (prevStripe !== rawStripeProp) {
+  const [inputs, setInputs] = React.useState({ rawStripe: rawStripeProp, options: optionsProp })
+  const { rawStripe, options } = inputs


nit: I find it confusing to remember the difference between rawStripe/rawStripeProp, options/optionsProp etc. How about we:

Remove the destructuring of the Elements function parameter and just name that parameter props

Rename the [inputs, setInputs] state variables to [savedProps, setSavedProps]

Refer to the various values everywhere as props.stripe / savedProps.stripe, props.options / savedProps.options, etc.

What do you think? Does that seem clearer?

Totally agree with you! I thought it was so customary 😁. But IMO it's better to name inputs, because it's some kind of input to construct context and if it's named props it should contain children as well.

christopher-stripe · 2020-05-20T17:12:13Z

src/components/Elements.tsx

-  }
+  const maybeStripe = usePromiseResolver(rawStripe)
+  const stripe = validateStripe(maybeStripe)
+  const [ctx, setContext] = React.useState<ElementsContextValue>(() => createElementsContext(null));


blocking: I think this will cause a subtle change in behavior that may break existing integrations. Consider someone using React Stripe.js like this:

// Synchronously create `Stripe` instance at module level const stripe = Stripe('pk_123') const CheckoutPage = () => { // Pass already instantiated `Stripe` instance to `Elements` provider return ( <Elements stripe={stripe}> <CheckoutForm /> </Elements> ) } const CheckoutForm = () => { // Integration assumes `stripe` is not `null` on first render const stripe = useStripe(); const [paymentRequest] = useState(() => stripe.paymentRequest(options)) // ...etc. }

This integration expects that if a valid Stripe instance is passed to an Elements provider, that Stripe instance is available via useStripe in a child component when the child component first renders. With the current behavior in this PR, the child component will be rendered twice and useStripe will return null during the first render.

Here's a failing test for this:

it('exposes a passed Stripe instance immediately to children calling useStripe', () => { const TestChild = () => { const stripe = useStripe(); if (stripe === null) { throw new Error('TestChild rendered with null stripe instance'); } return null; }; expect(() => { mount( <Elements stripe={stripe}> <TestChild /> </Elements> ); }).not.toThrow(); });

I also thought about this. But declined by 2 reasons:

The interface defines nullable values.

The existing implementation also doesn't guarantee stripe instance on mount. As no one guarantees that the state will be updated before the first render of children ;-)

Anyway it's easy to add.

Confirming - the existing code will provide empty values on the first render as well.

christopher-stripe · 2020-05-20T17:16:04Z

src/utils/usePromiseResolver.ts

+import React from 'react';
+import {isPromise} from '../utils/guards';
+
+export const usePromiseResolver = <T>(mayBePromise: T | PromiseLike<T>): T | null => {


nit: in the return value of this hook, there's no way to distinguish between a promise that resolved with null and a promise that has not yet resolved. It doesn't matter based on how this hook has been used in the Elements provider above, but I worry that this will be confusing in the future since it seems like this hook intends to be a general purpose utility.

How about changing the signature of this hook to be something like

<T>(maybePromise: T | PromiseLike<T>) => {settled: false} | {settled: true, value: T}

I think we can go deeper and provide full promise state ;-)

src/utils/usePromiseResolver.test.tsx

christopher-stripe · 2020-05-20T17:27:36Z

src/utils/usePromiseResolver.ts

+    setResolved(null)
+    mayBePromise.then(resolvedValue => {
+      if (isMounted) setResolved(resolvedValue)
+    }, () => undefined)


question: why add () => undefined as the rejection handler? Don't we want to know if a promise is rejected?

It depends on the hook purpose. If it's generic purpose hook, it should provide, otherwise in case of what provider does, it doesn't make sense as provider doesn't do anything with a rejection.

khmm12 · 2020-05-20T19:15:09Z

Hey @christopher-stripe!

Thank for your feedback! I left some updates based on it.

In addition to my inline comments, I have one more blocking comment: given that the general idea of these changes is to make React Stripe.js work with Strict Mode, could you add a test to that effect? For example, showing that a basic React Stripe.js integration doesn't error if it's wrapped with <React.StrictMode>.

This is the first thing I did before changing anything 😁.But the main problem seems enzyme doesn't work strict mode. It doesn't update react context.

christopher-stripe · 2020-06-01T16:21:47Z

This is the first thing I did before changing anything 😁.But the main problem seems enzyme doesn't work strict mode. It doesn't update react context.

I felt it was important to have some tests like this, so I went ahead and updated the whole suite to use React Testing Library instead of Enzyme (#97).

Could you merge or rebase against master to use the new tests? I also added a few tests for Strict Mode that are currently skipped (test.skip). Could you enable those?

This might cause some gnarly merge conflicts, so sorry for that. If you want me to take over this PR and handle getting it merged, I would be happy to—just let me know!

khmm12 · 2020-06-08T18:59:05Z

Hey @christopher-stripe 👋

I felt it was important to have some tests like this, so I went ahead and updated the whole suite to use React Testing Library instead of Enzyme

Good choice! Indeed nowadays react-testing-library is more popular than enzyme.

Could you merge or rebase against master to use the new tests? I also added a few tests for Strict Mode that are currently skipped (test.skip). Could you enable those?

I enabled 1 from 2 tests and added one more (you can try it against the master).

react-hooks-testing-library (unlike of react-testing-library) uses react-test-renderer
which implements light version of react-dom reconciler. Both StrictMode and ConcurrentMode are noticeable for react-dom only. So to test anything related to StrictMode or ConcurrentMode needs to use either react-dom directly or any testing-library with react-dom underhood (react-testing-library does). This is exactly what react team is doing (proof)
In StrictMode react calls all hook factories twice. That means the factory there (76533dc#diff-0acd678be951c54a972a22fd99f082eeR120) is called twice on component mount (in StrictMode only). This is another reason why I didn't add initialization on mount and still would avoid. If elements initialization produces any side-effects it should be done inside useEffect as I initially proposed, because this is only one predictable recommended way to produce any side-effects.

Strict mode can’t automatically detect side effects for you, but it can help you spot them by making them a little more deterministic. This is done by intentionally double-invoking the following functions

https://reactjs.org/docs/strict-mode.html

khmm12 · 2020-06-08T19:22:14Z

@christopher-stripe

fb49dd3

If you have any concerns regarding to elements, it can be postponed to next major version.

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 facebook/react#18003 facebook/react#18545

christopher-stripe · 2020-06-16T21:20:05Z

Thanks @khmm12. I'm slowly realizing how hard it would be to preserve the existing behavior while also supporting Strict Mode. I talked this over with some of my colleagues at Stripe, and we feel that:

We don't want to change the existing initialization on mount / single render behavior without a major version bump
We don't want to bump the major version if this is the only breaking change

So we're planning to hold off Strict Mode support until we have other changes that would warrant a major release.

I'm going to close this PR for now. In any event, it's been very helpful to see exactly what it would take to support Strict Mode. This will be a helpful foundation to build off in the not-so-distant future when we revisit Strict Mode support.

theKashey · 2020-06-17T01:22:29Z

what about releasing strict-mode-compatible version as a separate entrypoint? Like react-stripe-js/strict
what about adding a little code for detecting StrictMode driven error condition and displaying some actionable error (at least point to the issue). It's easy to do it "duplicate" state with a ref - https://codesandbox.io/s/affectionate-cohen-rhzfl?file=/src/index.js

khmm12 · 2020-06-17T07:19:18Z

Hey @christopher-stripe!

I still don't understand why we can't merge the PR now, because we can revert effect-safe elements initialization (fb49dd3) and deliver it in major version later.

Without it doesn't change the existing behaviour. And even more it's more spec compliant than the current implementation.

Thanks @khmm12. I'm slowly realizing how hard it would be to preserve the existing behavior while also supporting Strict Mode.

Simply to revert effect-safe elements initialization.

khmm12 commented May 14, 2020

View reviewed changes

christopher-stripe self-requested a review May 14, 2020 21:45

christopher-stripe reviewed May 20, 2020

View reviewed changes

khmm12 force-pushed the fix/initialization-in-strict-concurrent-mode branch from 58fad83 to 73bfa3d Compare May 20, 2020 19:02

christopher-stripe mentioned this pull request May 27, 2020

Migrate to React Testing Library #97

Merged

khmm12 force-pushed the fix/initialization-in-strict-concurrent-mode branch 2 times, most recently from 732b846 to 581499c Compare June 8, 2020 18:35

khmm12 force-pushed the fix/initialization-in-strict-concurrent-mode branch from bd35e6d to 1648083 Compare June 8, 2020 19:19

khmm12 added 2 commits June 8, 2020 23:59

Initialize stripe.elements on component did mount

fb49dd3

khmm12 force-pushed the fix/initialization-in-strict-concurrent-mode branch from 1648083 to fb49dd3 Compare June 8, 2020 20:59

christopher-stripe mentioned this pull request Jun 9, 2020

Not compatible with StrictMode #100

Closed

christopher-stripe closed this Jun 16, 2020

Fix Elements initialization in React Strict/Concurrent mode #93

Fix Elements initialization in React Strict/Concurrent mode #93

Uh oh!

Conversation

khmm12 commented May 14, 2020

Summary & motivation

Testing & documentation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopher-stripe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khmm12 commented May 20, 2020

Uh oh!

christopher-stripe commented Jun 1, 2020

Uh oh!

khmm12 commented Jun 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

khmm12 commented Jun 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christopher-stripe commented Jun 16, 2020

Uh oh!

theKashey commented Jun 17, 2020

Uh oh!

khmm12 commented Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

khmm12 commented Jun 8, 2020 •

edited

Loading

khmm12 commented Jun 8, 2020 •

edited

Loading

khmm12 commented Jun 17, 2020 •

edited

Loading