Flush discrete updates immediately in a batched context

[eps1lon]

Summary

Given:

act(() => {
  button.click();
  button.click();
})

Previously each update from the click was batched inside act. However, in production React will never batch updates from user clicks since these are "discrete" events. With this PR discrete updates will be flushed immediately instead of being batched.

The previous behavior resulted in bugs like #20074 not being detected via automated testing. In Material-UI we already need extra fireDiscreteEvent.click testing utilities that make sure click events are flushed immediately from inside act to test certain browser behavior.

Test Plan

• CI green
• test build in testing-library/react-testing-library: NOT FOR MERGE test: run with a version of act that flushes discrete events imeadiately  testing-library/react-testing-library#896
• test build in mui-org/material-ui: NOT FOR MERGE test: run with a version of act that flushes discrete events imeadiately  mui/material-ui#25658

[eps1lon]

I don't agree that we shouldn't flush inside act. act was supposed to test closer to what a user sees. Batching discrete updates is not something a user can ever see.

Some history of the comment:

• changed: 73c380f
• introduced: ef4ac42#diff-4795b2bc8a2ac82f7c16985975dffe52bb4435555755f321f9ab7d76f5b48807R571-R572

Looks like there was never a follow-up mentioned when introducing this comment.

Pinging @acdlite since he changed the comment most recently.

[sizebot]

Comparing: 7841d06...579bdc3

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	126.00 kB	126.00 kB	=	40.41 kB	40.41 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	128.82 kB	128.82 kB	=	41.35 kB	41.35 kB
facebook-www/ReactDOM-prod.classic.js	=	406.09 kB	406.09 kB	=	75.11 kB	75.11 kB
facebook-www/ReactDOM-prod.modern.js	=	394.46 kB	394.46 kB	=	73.29 kB	73.29 kB
facebook-www/ReactDOMForked-prod.classic.js	=	406.09 kB	406.09 kB	=	75.11 kB	75.11 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 579bdc3

[eps1lon]

In my opinion, this was undesired behavior. A user clicking three times would see all three updates being flushed not just one. Programmatic click via DOM API would also flush three times: https://codesandbox.io/s/click-is-not-batched-re61e?file=/src/index.js:587-634

[gaearon]

I added this to our project checklist to review when we get to other act changes.

[sebmarkbage]

in production React will never batch updates from clicks

Note that this is not quite true:

https://github.com/facebook/react/pull/21223/files#diff-f734c325f6eb23cd0f816b11eac3c6437844de0db32c0839f0d32dde7a2c3124

Use of element.click() multiple times in one browser event would be batched. act works roughly as a browser task assuming you wait long enough to observe it. So technically this is testing what would happen in this scenario: onClick={() => { element.click(); element.click(); }}

It might not be helpful in practice though so maybe it should instead be a warning to add act around each click.

[eps1lon]

Thanks for the clarification. I didn't consider that scenario.

So technically this is testing what would happen in this scenario: onClick={() => { element.click(); element.click(); }}

click() is probably a bad example here since that seems like an exotic use case. focus() is more likely to happen in discrete event handlers such as keyDown while also being the target of tests.

So I do understand that wrapping discrete even dispatches in act does make sense if we want to simulate how these events would be treated in a react event handler.

However, this is not what act was promoted as.

This helps make your tests run closer to what real users would experience when using your application. The rest of these examples use act() to make these guarantees.

-- https://reactjs.org/docs/testing-recipes.html#act

act(() => element.click()) would not simulate a user click.

It's not clear to me why we'd want to test how a discrete event in another event handler would behave. In that case why not dispatch the actual event? For example

function MenuItem() {
  return <button onKeyDown={(event) => event.currentTarget.blur()} role="menuitem" />
}
render(<MenuItem />)
const menuitem = document.querySelector('[role="menuitem"]');

act(() => {
-	menuitem.blur();
+	menuitem.dispatchEvenet(new KeyboardEvent('keydown', { bubbles: true, cancelable: true }));
})

Ideally I could take a look at every usage of fireEvent.click of @testing-library/react and check if that is testing a click from an event handler or a user click. Though I'd suspect that almost all of these clicks are supposed to be user events not events from within another handler.

It might not be helpful in practice though so maybe it should instead be a warning to add act around each click.

I'm tempted to try a version of @testing-library/react where we don't wrap discrete event dispatches in act (which we currently do). Would this still trigger "missing act() warnings"?

[eps1lon]

act() behavior and its interaction with discrete events changed a bit since I've openend it. There are various little suprises I've encountered when trying concurrent react in the latest experimental build. I'm going to compile these instead because it's not clear to me how the testing story is supposed to look now.

[eps1lon]

Closing since it's cleared up for me now.