Fix native event batching in concurrent mode (#21010)

* 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 <git@andrewclark.io>
facebook · Mar 16, 2021 · 89acfa6 · 89acfa6
1 parent 0203b65
commit 89acfa6
Show file tree

Hide file tree

Showing 4 changed files with 70 additions and 16 deletions.
diff --git a/packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js b/packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js
@@ -201,32 +201,32 @@ describe('React hooks DevTools integration', () => {
     if (__DEV__) {
       // First render was locked
       expect(renderer.toJSON().children).toEqual(['Loading']);
-      scheduleUpdate(fiber); // Re-render
+      act(() => scheduleUpdate(fiber)); // Re-render
       expect(renderer.toJSON().children).toEqual(['Loading']);
 
       // Release the lock
       setSuspenseHandler(() => false);
-      scheduleUpdate(fiber); // Re-render
+      act(() => scheduleUpdate(fiber)); // Re-render
       expect(renderer.toJSON().children).toEqual(['Done']);
-      scheduleUpdate(fiber); // Re-render
+      act(() => scheduleUpdate(fiber)); // Re-render
       expect(renderer.toJSON().children).toEqual(['Done']);
 
       // Lock again
       setSuspenseHandler(() => true);
-      scheduleUpdate(fiber); // Re-render
+      act(() => scheduleUpdate(fiber)); // Re-render
       expect(renderer.toJSON().children).toEqual(['Loading']);
 
       // Release the lock again
       setSuspenseHandler(() => false);
-      scheduleUpdate(fiber); // Re-render
+      act(() => scheduleUpdate(fiber)); // Re-render
       expect(renderer.toJSON().children).toEqual(['Done']);
 
       // Ensure it checks specific fibers.
       setSuspenseHandler(f => f === fiber || f === fiber.alternate);
-      scheduleUpdate(fiber); // Re-render
+      act(() => scheduleUpdate(fiber)); // Re-render
       expect(renderer.toJSON().children).toEqual(['Loading']);
       setSuspenseHandler(f => f !== fiber && f !== fiber.alternate);
-      scheduleUpdate(fiber); // Re-render
+      act(() => scheduleUpdate(fiber)); // Re-render
       expect(renderer.toJSON().children).toEqual(['Done']);
     } else {
       expect(renderer.toJSON().children).toEqual(['Done']);
@@ -259,33 +259,33 @@ describe('React hooks DevTools integration', () => {
     if (__DEV__) {
       // First render was locked
       expect(renderer.toJSON().children).toEqual(['Loading']);
-      scheduleUpdate(fiber); // Re-render
+      act(() => scheduleUpdate(fiber)); // Re-render
       expect(renderer.toJSON().children).toEqual(['Loading']);
 
       // Release the lock
       setSuspenseHandler(() => false);
-      scheduleUpdate(fiber); // Re-render
+      act(() => scheduleUpdate(fiber)); // Re-render
       Scheduler.unstable_flushAll();
       expect(renderer.toJSON().children).toEqual(['Done']);
-      scheduleUpdate(fiber); // Re-render
+      act(() => scheduleUpdate(fiber)); // Re-render
       expect(renderer.toJSON().children).toEqual(['Done']);
 
       // Lock again
       setSuspenseHandler(() => true);
-      scheduleUpdate(fiber); // Re-render
+      act(() => scheduleUpdate(fiber)); // Re-render
       expect(renderer.toJSON().children).toEqual(['Loading']);
 
       // Release the lock again
       setSuspenseHandler(() => false);
-      scheduleUpdate(fiber); // Re-render
+      act(() => scheduleUpdate(fiber)); // Re-render
       expect(renderer.toJSON().children).toEqual(['Done']);
 
       // Ensure it checks specific fibers.
       setSuspenseHandler(f => f === fiber || f === fiber.alternate);
-      scheduleUpdate(fiber); // Re-render
+      act(() => scheduleUpdate(fiber)); // Re-render
       expect(renderer.toJSON().children).toEqual(['Loading']);
       setSuspenseHandler(f => f !== fiber && f !== fiber.alternate);
-      scheduleUpdate(fiber); // Re-render
+      act(() => scheduleUpdate(fiber)); // Re-render
       expect(renderer.toJSON().children).toEqual(['Done']);
     } else {
       expect(renderer.toJSON().children).toEqual(['Done']);

diff --git a/packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js b/packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js
@@ -281,4 +281,52 @@ describe('ReactDOMNativeEventHeuristic-test', () => {
       expect(container.textContent).toEqual('hovered');
     });
   });
+
+  // @gate experimental
+  it('should batch inside native events', async () => {
+    const root = ReactDOM.unstable_createRoot(container);
+
+    const target = React.createRef(null);
+    function Foo() {
+      const [count, setCount] = React.useState(0);
+      const countRef = React.useRef(-1);
+
+      React.useLayoutEffect(() => {
+        countRef.current = count;
+        target.current.onclick = () => {
+          setCount(countRef.current + 1);
+          // Now update again. If these updates are batched, then this should be
+          // a no-op, because we didn't re-render yet and `countRef` hasn't
+          // been mutated.
+          setCount(countRef.current + 1);
+        };
+      });
+      return <div ref={target}>Count: {count}</div>;
+    }
+
+    await act(async () => {
+      root.render(<Foo />);
+    });
+    expect(container.textContent).toEqual('Count: 0');
+
+    // Ignore act warning. We can't use act because it forces batched updates.
+    spyOnDev(console, 'error');
+
+    const pressEvent = document.createEvent('Event');
+    pressEvent.initEvent('click', true, true);
+    dispatchAndSetCurrentEvent(target.current, pressEvent);
+    // If this is 2, that means the `setCount` calls were not batched.
+    expect(container.textContent).toEqual('Count: 1');
+
+    // Assert that the `act` warnings were the only ones that fired.
+    if (__DEV__) {
+      expect(console.error).toHaveBeenCalledTimes(2);
+      expect(console.error.calls.argsFor(0)[0]).toContain(
+        'was not wrapped in act',
+      );
+      expect(console.error.calls.argsFor(1)[0]).toContain(
+        'was not wrapped in act',
+      );
+    }
+  });
 });
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -547,7 +547,10 @@ export function scheduleUpdateOnFiber(
     } else {
       ensureRootIsScheduled(root, eventTime);
       schedulePendingInteractions(root, lane);
-      if (executionContext === NoContext) {
+      if (
+        executionContext === NoContext &&
+        (fiber.mode & ConcurrentMode) === NoMode
+      ) {
         // Flush the synchronous work now, unless we're already working or inside
         // a batch. This is intentionally inside scheduleUpdateOnFiber instead of
         // scheduleCallbackForFiber to preserve the ability to schedule a callback

diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
@@ -547,7 +547,10 @@ export function scheduleUpdateOnFiber(
     } else {
       ensureRootIsScheduled(root, eventTime);
       schedulePendingInteractions(root, lane);
-      if (executionContext === NoContext) {
+      if (
+        executionContext === NoContext &&
+        (fiber.mode & ConcurrentMode) === NoMode
+      ) {
         // Flush the synchronous work now, unless we're already working or inside
         // a batch. This is intentionally inside scheduleUpdateOnFiber instead of
         // scheduleCallbackForFiber to preserve the ability to schedule a callback