From 0120986c8401605a41acbcfa37327c6cfb0966c8 Mon Sep 17 00:00:00 2001 From: Rick Hanlon Date: Mon, 15 Mar 2021 18:46:59 -0400 Subject: [PATCH 1/4] Fix native event batching in concurrent mode --- .../ReactDOMNativeEventHeuristic-test.js | 40 +++++++++++++++++++ .../src/ReactFiberWorkLoop.new.js | 5 ++- .../src/ReactFiberWorkLoop.old.js | 5 ++- 3 files changed, 48 insertions(+), 2 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js b/packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js index dd682d2def59f..bbd883adcdb81 100644 --- a/packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js @@ -281,4 +281,44 @@ 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); + + React.useEffect(() => { + Scheduler.unstable_yieldValue(count); + }, [count]); + + React.useLayoutEffect(() => { + target.current.onclick = () => { + setCount(c => c + 1); + // Now update again, this should be batched. + setCount(c => c + 1); + }; + }); + return
Count: {count}
; + } + + await act(async () => { + root.render(); + }); + expect(Scheduler).toHaveYielded([0]); + 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); + + expect(Scheduler).toHaveYielded([]); + expect(container.textContent).toEqual('Count: 2'); + expect(Scheduler).toFlushAndYield([2]); + }); }); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 926c1d0b28acc..fa3d4f070a2fc 100644 --- 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 ( + (fiber.mode & ConcurrentMode) === NoMode && + executionContext === NoContext + ) { // 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 index 6450ac710d7ed..580258ae9d994 100644 --- 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 ( + (fiber.mode & ConcurrentMode) === NoMode && + executionContext === NoContext + ) { // 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 From ef8ac8cae995e0836fa73ee907909ddf23783af4 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 15 Mar 2021 21:38:51 -0500 Subject: [PATCH 2/4] 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. --- .../ReactDevToolsHooksIntegration-test.js | 28 +++++++++---------- .../ReactDOMNativeEventHeuristic-test.js | 11 ++++++-- 2 files changed, 23 insertions(+), 16 deletions(-) diff --git a/packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js b/packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js index 6f9cf52787e13..3eea121d65b36 100644 --- 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 index bbd883adcdb81..10b9212a19c77 100644 --- a/packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js @@ -317,8 +317,15 @@ describe('ReactDOMNativeEventHeuristic-test', () => { pressEvent.initEvent('click', true, true); dispatchAndSetCurrentEvent(target.current, pressEvent); - expect(Scheduler).toHaveYielded([]); expect(container.textContent).toEqual('Count: 2'); - expect(Scheduler).toFlushAndYield([2]); + if (gate(flags => flags.enableDiscreteEventFlushingChange)) { + // When enableDiscreteEventFlushingChange is enabled, we don't flush + // discrete callbacks synchronously at the end of the event. + // TODO: I believe this flag is no longer relevant, because we flush the + // updates in a microtask, anyway. + expect(Scheduler).toFlushAndYield([2]); + } else { + expect(Scheduler).toHaveYielded([2]); + } }); }); From 632dedef1f0749c6aa04758fe5753cda665b4364 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 15 Mar 2021 22:10:19 -0500 Subject: [PATCH 3/4] Testing nits --- .../ReactDOMNativeEventHeuristic-test.js | 37 ++++++++++--------- 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js b/packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js index 10b9212a19c77..c0617f9b56787 100644 --- a/packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js @@ -289,16 +289,16 @@ describe('ReactDOMNativeEventHeuristic-test', () => { const target = React.createRef(null); function Foo() { const [count, setCount] = React.useState(0); - - React.useEffect(() => { - Scheduler.unstable_yieldValue(count); - }, [count]); + const countRef = React.useRef(-1); React.useLayoutEffect(() => { + countRef.current = count; target.current.onclick = () => { - setCount(c => c + 1); - // Now update again, this should be batched. - setCount(c => c + 1); + 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
Count: {count}
; @@ -307,7 +307,6 @@ describe('ReactDOMNativeEventHeuristic-test', () => { await act(async () => { root.render(); }); - expect(Scheduler).toHaveYielded([0]); expect(container.textContent).toEqual('Count: 0'); // Ignore act warning. We can't use act because it forces batched updates. @@ -316,16 +315,18 @@ describe('ReactDOMNativeEventHeuristic-test', () => { const pressEvent = document.createEvent('Event'); pressEvent.initEvent('click', true, true); dispatchAndSetCurrentEvent(target.current, pressEvent); - - expect(container.textContent).toEqual('Count: 2'); - if (gate(flags => flags.enableDiscreteEventFlushingChange)) { - // When enableDiscreteEventFlushingChange is enabled, we don't flush - // discrete callbacks synchronously at the end of the event. - // TODO: I believe this flag is no longer relevant, because we flush the - // updates in a microtask, anyway. - expect(Scheduler).toFlushAndYield([2]); - } else { - expect(Scheduler).toHaveYielded([2]); + // 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', + ); } }); }); From 39ea57671dad5a5a174a67f83720e5926efd8e00 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 15 Mar 2021 22:12:04 -0500 Subject: [PATCH 4/4] Nit: Check executionContext === NoContext first In the common case it will be false and the binary expression will short circuit. --- packages/react-reconciler/src/ReactFiberWorkLoop.new.js | 4 ++-- packages/react-reconciler/src/ReactFiberWorkLoop.old.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index fa3d4f070a2fc..19d53553f43d8 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -548,8 +548,8 @@ export function scheduleUpdateOnFiber( ensureRootIsScheduled(root, eventTime); schedulePendingInteractions(root, lane); if ( - (fiber.mode & ConcurrentMode) === NoMode && - executionContext === NoContext + 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 diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 580258ae9d994..da22638e75758 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -548,8 +548,8 @@ export function scheduleUpdateOnFiber( ensureRootIsScheduled(root, eventTime); schedulePendingInteractions(root, lane); if ( - (fiber.mode & ConcurrentMode) === NoMode && - executionContext === NoContext + 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