Bug: state could be reset into the initialValue when using StrictMode

[theKashey]

React version: 16.13.1

Steps To Reproduce

1. Wrap application with StrictMode
2. use useState
3. sometimes value would be reset to the initial

Link to code example: https://codesandbox.io/s/react-stripe-js-z79qw?file=/src/index.js

There are two cases, one is not working in StrictMode and one works well. Without strick mode both works. Console output is empty.

We actually found the buggy behaviour in stripe-js/react - it uses useState to save the context value.
And it is (not) working this way:

• (render1) the stripe value set
• (render1) setContext is called (https://github.com/stripe/react-stripe-js/blob/c97c0ee094605c1d6aa1528f740e7c784e2464a7/src/components/Elements.tsx#L135)
• (render2) component rerendered with a correct state
• (render3) component rerendered with incorrect state, equal to the initial one

In the same time:

• data in "refs" is correct
• using a bit different API (promises), moving setContext update into nested component, is working as well. (https://github.com/stripe/react-stripe-js/blob/c97c0ee094605c1d6aa1528f740e7c784e2464a7/src/components/Elements.tsx#L148)

The current behavior

I've failed to understand the current behavior - there is a case (we know one) when state is reset, while there are many cases where everything is working as expected.

The expected behavior

State should be always preserved; StrictMode should have to effect on hooks.

[vkurchatkin]

The idea of StrictMode that it double renders (among other things) all components trying to catch some unintended side-effects. So, if something works without StrictMode, but breaks with it then here you go: you just found a bug in your code! (or in your case, in react-stripe-js)

[theKashey]

Before opening this issue I was reading thought existing issues, and there were many about double rendering. And look like it is a root case! Thank you for pointing on it yet again.

  if (!final.current) {
    if (parsed.tag === 'sync') {
      final.current = true; // 👈 this will prevent `setContext` to be called "second time"
      setContext({
        stripe: parsed.stripe,
        elements: parsed.stripe.elements(options),
      });
    }

In other words - React will render component once, and it will do it "properly", and then it will render it second time, but updated ref will amend the behavior, and state will not be updated. And that's why promise based code is working well - it's just "out of double rendering cycle".

However this means two things:

• react is not reporting about the issue with Strict it might discovered.
• StrictMode is not "refs-compatible", and it's not safe to use it.

👆 and that sounds like a really big bug, as long as refs are first-class citizens.

[vkurchatkin]

StrictMode is not "refs-compatible", and it's not safe to use it.

StrictMode is "refs-compatible", but you need to follow the rules around refs, e.g. not mutating them in render (except for lazy initialization)

react is not reporting about the issue with Strict it might discovered.

Unfortunately, that's not possible. StrictMode doesn't have any idea it has discovered anything. It is up to the developer to figure that out.

[theKashey]

Isn't it supposed to throw some warnings, not break an application without any notice?

In any case probably the root cause is calling setState from a render, not useEffect, which probably was made to allow only one call, and useReducer would solve that issue in a better way.

Probably that's why the rest our Application is working - we do not using update-in-render pattern stripe has used.

[vkurchatkin]

Isn't it supposed to throw some warnings, not break an application without any notice?

Is shows warning when you are using deprecated APIs and such. The idea of double-invoking is exactly that it might break something without any notice.

[eps1lon]

Isn't it supposed to throw some warnings, not break an application without any notice?

The page uses different wording for side effect issues. If it could detect them, it would use the same wording:

Strict mode can’t automatically detect side effects for you, but it can help you spot them by making them a little more deterministic.

-- https://reactjs.org/docs/strict-mode.html#detecting-unexpected-side-effects

[theKashey]

But one could monkey patch useState and detect situations like this. Gonna check my code for other place, which I might missed...

Working example: https://codesandbox.io/s/affectionate-cohen-rhzfl?file=/src/index.js

[eps1lon]

But one could monkey patch useState and detect situations like this.

You don't know where this value comes from. It could be safe e.g. from a mutable source or context. React itself might be able to do that because it could know that a second setState was triggered from strict mode.

But in concurrent mode it's valid to call setState with different values between renders.

[theKashey]

Well, one should use concurrent mode only after a dozen of checks, StrickMode included, completely passed. So monkeypatching some stuff might help a lot, and then be completely removed till next time.

[theKashey]

Anyway, look like this is not a bug, but a feature. I consider my question as fully answered and the problem is absolutely clear now.
It's not only "not right", but actually "not safe" to base any logic on refs. If one could do it other, probably immutable way,- they should.