Fix native event batching in concurrent mode #21010

rickhanlonii · 2021-03-15T22:51:20Z

Overview

This PR fixes a bug exposed by #20968 where we were running a flush for legacy mode behavior in concurrent mode.

rickhanlonii · 2021-03-15T22:51:46Z

packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js

+    expect(container.textContent).toEqual('Count: 0');
+
+    // Ignore act warning. We can't use act because it forces batched updates.
+    spyOnDev(console, 'error');


I don't love this but I cannot get toErrorDev to work.

Make sure you assert that only the expected error is logged, though

expect(console.error).toHaveBeenCalledTimes(1); expect(console.error.calls.argsFor(0)[0]).toContain(hookWarningMessage);

sizebot · 2021-03-15T22:56:06Z

Comparing: f8979e0...39ea576

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	+0.02%	122.83 kB	122.85 kB	+0.04%	39.52 kB	39.54 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	+0.02%	129.33 kB	129.35 kB	+0.03%	41.59 kB	41.60 kB
facebook-www/ReactDOM-prod.classic.js	=	408.70 kB	408.73 kB	=	75.80 kB	75.80 kB
facebook-www/ReactDOM-prod.modern.js	=	396.96 kB	396.99 kB	=	73.86 kB	73.86 kB
facebook-www/ReactDOMForked-prod.classic.js	=	408.71 kB	408.74 kB	=	75.80 kB	75.80 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 39ea576

acdlite · 2021-03-15T22:57:48Z

packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js

+
+      React.useLayoutEffect(() => {
+        target.current.onclick = () => {
+          setCount(c => c + 1);


If you change these to setCount(c + 1) instead of using the updater form, then the final DOM output will be different depending on whether they are batched. 1 if batched, 2 if not batched.

Edit: Or rather, setCount(someRef.current + 1).

Just a suggestion because the test failure will be more obvious. Like if you ran this in a sandbox, you wouldn't notice if there were two synchronous commits instead of one, but you would notice that it's displaying the wrong number.

acdlite · 2021-03-15T22:59:49Z

packages/react-reconciler/src/ReactFiberWorkLoop.old.js

-      if (executionContext === NoContext) {
+      if (
+        (fiber.mode & ConcurrentMode) === NoMode &&
+        executionContext === NoContext


Nit: Check executionContext === NoContext first because in the common case it will be false and the binary expression will short circuit.

rickhanlonii · 2021-03-16T01:52:02Z

@acdlite looks like tests are failing because we also do some sync flushing here as well. I'm going to need some more time to look into that.

These tests expect the `scheduleUpdate` DevTools hook to trigger a synchronous re-render with legacy semantics, but flushing in a microtask is fine. Wrapping the updates with `act` fixes it.

acdlite · 2021-03-16T02:53:14Z

There was a DevTools hooks integration test that was failing because it happened to rely on legacy sync flushing behavior. I fixed by wrapping in act.

The only remaining test that is failing is the one you just added. It only fails when enableDiscreteEventFlushingChange is on, which is called inside flushDiscreteUpdatesIfNeeded.

Since we're moving to a model where we always flush discrete updates at the end of the event, I think we can safely remove that flag. I might be wrong, but even if I am — we don't have to decide in this PR. Update your test so that it passes in either branch. (EDIT: I went ahead and pushed this change.)

I added an item to the backlog to figure out what to do with flushDiscreteUpdatesIfNeeded.

In the common case it will be false and the binary expression will short circuit.

rickhanlonii · 2021-03-16T13:55:19Z

Actually, that's not the culprit, this is:

react/packages/react-dom/src/events/ReactDOMEventListener.js

Lines 175 to 182 in e4d4b70

    
           if ( 
        
             !enableLegacyFBSupport || 
        
             // If we are in Legacy FB support mode, it means we've already 
        
             // flushed for this event and we don't need to do it again. 
        
             (eventSystemFlags & IS_LEGACY_FB_SUPPORT_MODE) === 0 
        
           ) { 
        
             flushDiscreteUpdatesIfNeeded(nativeEvent.timeStamp); 
        
           }

acdlite · 2021-03-16T14:23:50Z

Regardless that’s not a blocker

acdlite · 2021-03-16T14:31:16Z

Also that’s the same code path but one caller above in the stack. In the open source build (where it was failing) enableLegacyFBSupport is off, so all it does is call flushDiscreteUpdatesIfNeeded. And since we’re getting rid of the time stamp heuristic, in favor of always flushing immediately, the whole thing can effectively turn into a flushSync call.

* Fix native event batching in concurrent mode * Wrap DevTools test updates with act These tests expect the `scheduleUpdate` DevTools hook to trigger a synchronous re-render with legacy semantics, but flushing in a microtask is fine. Wrapping the updates with `act` fixes it. * Testing nits * Nit: Check executionContext === NoContext first In the common case it will be false and the binary expression will short circuit. Co-authored-by: Andrew Clark <[email protected]>

Summary: This sync includes the following changes: - **[6d3ecb70d](facebook/react@6d3ecb70d )**: Remove unstable_changedBits ([#20953](facebook/react#20953)) //<Andrew Clark>// - **[754e30728](facebook/react@754e30728 )**: Delete immediateQueueCallbackNode ([#20980](facebook/react#20980)) //<Andrew Clark>// - **[be5a2e231](facebook/react@be5a2e231 )**: Land enableSyncMicrotasks ([#20979](facebook/react#20979)) //<Andrew Clark>// - **[f6bc9c824](facebook/react@f6bc9c824 )**: [Fizz] Expose maxBoundarySize as an option ([#21029](facebook/react#21029)) //<Sebastian Markbåge>// - **[154b85213](facebook/react@154b85213 )**: [Fizz] Expose a method to explicitly start writing to a Node stream ([#21028](facebook/react#21028)) //<Sebastian Markbåge>// - **[cf485e6f6](facebook/react@cf485e6f6 )**: [Fizz] Expose a method to abort a pending request ([#21027](facebook/react#21027)) //<Sebastian Markbåge>// - **[3fb11eed9](facebook/react@3fb11eed9 )**: React Native: Touch Instrumentation Interface ([#21024](facebook/react#21024)) //<Timothy Yung>// - **[825c3021f](facebook/react@825c3021f )**: Don't delete trailing mismatches during hydration at the root ([#21021](facebook/react#21021)) //<Sebastian Markbåge>// - **[1d1e49cfa](facebook/react@1d1e49cfa )**: [Fizz] Assign an ID to the first DOM element in a fallback or insert a dummy (and testing infra) ([#21020](facebook/react#21020)) //<Sebastian Markbåge>// - **[466b26c92](facebook/react@466b26c92 )**: Store commit durations on HostRoot for DevTools access ([#20983](facebook/react#20983)) //<Brian Vaughn>// - **[89acfa639](facebook/react@89acfa639 )**: Fix native event batching in concurrent mode ([#21010](facebook/react#21010)) //<Ricky>// - **[0203b6567](facebook/react@0203b6567 )**: chore: update react-reconciler README ([#21016](facebook/react#21016)) //<susiwen8>// Changelog: [General][Changed] - React Native sync for revisions c9f6d0a...6d3ecb7 jest_e2e[run_all_tests] Reviewed By: JoshuaGross, kacieb Differential Revision: D27231625 fbshipit-source-id: 89c0c0662e69044ae8890486a693013bee6005dd

* Fix native event batching in concurrent mode * Wrap DevTools test updates with act These tests expect the `scheduleUpdate` DevTools hook to trigger a synchronous re-render with legacy semantics, but flushing in a microtask is fine. Wrapping the updates with `act` fixes it. * Testing nits * Nit: Check executionContext === NoContext first In the common case it will be false and the binary expression will short circuit. Co-authored-by: Andrew Clark <[email protected]>

Fix native event batching in concurrent mode

0120986

rickhanlonii requested a review from acdlite March 15, 2021 22:51

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 15, 2021

rickhanlonii commented Mar 15, 2021

View reviewed changes

acdlite reviewed Mar 15, 2021

View reviewed changes

acdlite approved these changes Mar 15, 2021

View reviewed changes

Wrap DevTools test updates with act

ef8ac8c

These tests expect the `scheduleUpdate` DevTools hook to trigger a synchronous re-render with legacy semantics, but flushing in a microtask is fine. Wrapping the updates with `act` fixes it.

acdlite force-pushed the rh-native-sync-bug branch from 3afc162 to ef8ac8c Compare March 16, 2021 03:00

acdlite added 2 commits March 15, 2021 22:10

Testing nits

632dede

Nit: Check executionContext === NoContext first

39ea576

In the common case it will be false and the binary expression will short circuit.

acdlite force-pushed the rh-native-sync-bug branch from fb173fc to 39ea576 Compare March 16, 2021 03:14

rickhanlonii merged commit 89acfa6 into facebook:master Mar 16, 2021

rickhanlonii deleted the rh-native-sync-bug branch March 16, 2021 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix native event batching in concurrent mode #21010

Fix native event batching in concurrent mode #21010

rickhanlonii commented Mar 15, 2021

rickhanlonii Mar 15, 2021

acdlite Mar 15, 2021

sizebot commented Mar 15, 2021 •

edited

Loading

acdlite Mar 15, 2021 •

edited

Loading

acdlite Mar 15, 2021

acdlite Mar 15, 2021

rickhanlonii commented Mar 16, 2021

acdlite commented Mar 16, 2021 •

edited

Loading

rickhanlonii commented Mar 16, 2021 •

edited

Loading

acdlite commented Mar 16, 2021

acdlite commented Mar 16, 2021

Fix native event batching in concurrent mode #21010

Fix native event batching in concurrent mode #21010

Conversation

rickhanlonii commented Mar 15, 2021

Overview

rickhanlonii Mar 15, 2021

Choose a reason for hiding this comment

acdlite Mar 15, 2021

Choose a reason for hiding this comment

sizebot commented Mar 15, 2021 • edited Loading

Critical size changes

Significant size changes

acdlite Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

acdlite Mar 15, 2021

Choose a reason for hiding this comment

acdlite Mar 15, 2021

Choose a reason for hiding this comment

rickhanlonii commented Mar 16, 2021

acdlite commented Mar 16, 2021 • edited Loading

rickhanlonii commented Mar 16, 2021 • edited Loading

acdlite commented Mar 16, 2021

acdlite commented Mar 16, 2021

sizebot commented Mar 15, 2021 •

edited

Loading

acdlite Mar 15, 2021 •

edited

Loading

acdlite commented Mar 16, 2021 •

edited

Loading

rickhanlonii commented Mar 16, 2021 •

edited

Loading