React: Bug: React's batching makes state comparison checks unreliable

[benoitgrelard]

It seems that in some situations React's batching makes comparison to change in state unreliable.
In the provided example, focus/blur handlers produce different outcomes:

• when triggered via tabbing
• vs. when manually focusing on keydown

React version: 17.0.0

Steps To Reproduce

1. Open the provided codesandbox
2. Focus the 2 inputs back and forth by tabbing
3. Note the onChange logs correspond to the updates and are in sync with the UI
4. Now instead, focus an input then press the down arrow to programmatically focus back and forth between the 2 inputs
5. Note the onChange logs are now wrong and inconsistent with the UI (initially true and then false forever)

Link to code example: https://codesandbox.io/s/react-staleness-ishfn?file=/src/App.js

The current behavior

When tabbing through this is the output I get:

onChange true
onChange false
onChange true
onChange false
onChange true
onChange false

When pressing arrow down to focus the inputs back and forth I get:

onChange true
onChange false
onChange false
onChange false
onChange false
onChange false

Note that if you check the "Prevent React from batching updates?" checkbox, both examples behave the same. The only difference is that in this case, we wrap the .focus() calls in a setTimeout() to avoid batching.

The expected behavior

We would expect the 2 examples to behave the same.

[sebmarkbage]

The question is, why does it matter? The terminal result is still the same.

There are some quirks that an effect can’t detect whether you temporarily set focus on the document and then back within the list of inputs.

This a fundamental issue with the browser’s event system because it models a temporarily incorrect state where the activeElement temporarily represents an incorrect state.

So I’d like to have both behave like the second one. Unfortunately there’s no trivial way to detect this properly in browsers. setTimeout might batch too many events where the previous change should’ve been observed in the next event handler.

There’s a possible way to listen to all capture events to know whether there will likely be a follow up one later.

But my reason for this might be different from yours. So please explain why this would matter in your use case?

Keep in mind that React can refire effects for other reasons too when things rerender.

[jjenzz]

The question is, why does it matter? The terminal result is still the same.

Yep, but we needed to know if a change had occurred. Our mistake here was to assume that the state would be an up to date reference to "change" when of course, that's not reliable given the way render closures work.

The first thing we tried was to use a ref instead which worked but we had convinced ourselves we were overlooking something here. Moral of the story is to not doubt ourselves so much next time 😅

I think we can close this. Thanks for the speedy reply @sebmarkbage !

[jjenzz]

@sebmarkbage We have looked into this some more. It definitely seems that there is a use-case that is not possible to get working here.

We have created another sandbox that describes the steps to reproduce and answers your “why does it matter?” question. We would be super grateful if you wouldn’t mind taking a look and giving us a hand here :slightly_smiling_face:

https://codesandbox.io/s/crimson-lake-gidmx?file=/src/App.js

Strangely, this issue only occurs when using onKeyDown. If you mouse move over the buttons instead, then things work as expected.

[eps1lon]

The workaround for this particular issue is similar to https://github.com/facebook/react/issues/18591:

React.useLayoutEffect(() => {
  ref.current = value;
});
....
<div onBlur={() => {
  ReactDOM.flushSync(() => {
    setValue(false)
  })
}}>

-- https://codesandbox.io/s/flushsync-semantics-eli4w?file=/src/App.js:1520-1580

Though it makes me wonder whether https://github.com/facebook/react/issues/18591 is not just related to concurrent mode. Intuitively I would not expect https://codesandbox.io/s/crimson-lake-gidmx?file=/src/App.js to work the way it does right now.

[jjenzz]

ReactDOM.flushSync! TIL 😮

If this is just a workaround, are we saying that it is something that will be fixed?

I'm not sure how we'll explain to our consumers that they need to do that 😅 Do we know when this predictably occurs and when this kind of fix is needed so we can maybe document it?

Thanks for the help @eps1lon