diff --git a/packages/react-devtools-shared/src/__tests__/storeStressTestConcurrent-test.js b/packages/react-devtools-shared/src/__tests__/storeStressTestConcurrent-test.js index 1090e24a0f735..38cd572ad6808 100644 --- a/packages/react-devtools-shared/src/__tests__/storeStressTestConcurrent-test.js +++ b/packages/react-devtools-shared/src/__tests__/storeStressTestConcurrent-test.js @@ -11,6 +11,7 @@ describe('StoreStressConcurrent', () => { let React; let ReactDOM; let act; + let actAsync; let bridge; let store; let print; @@ -23,6 +24,9 @@ describe('StoreStressConcurrent', () => { React = require('react'); ReactDOM = require('react-dom'); act = require('./utils').act; + // TODO: Figure out recommendation for concurrent mode tests, then replace + // this helper with the real thing. + actAsync = require('./utils').actAsync; print = require('./storeSerializer').print; }); @@ -758,7 +762,7 @@ describe('StoreStressConcurrent', () => { // Force fallback. expect(print(store)).toEqual(snapshots[i]); - act(() => { + await actAsync(async () => { bridge.send('overrideSuspense', { id: suspenseID, rendererID: store.getRendererIDForElement(suspenseID), @@ -768,7 +772,7 @@ describe('StoreStressConcurrent', () => { expect(print(store)).toEqual(snapshots[j]); // Stop forcing fallback. - act(() => { + await actAsync(async () => { bridge.send('overrideSuspense', { id: suspenseID, rendererID: store.getRendererIDForElement(suspenseID), @@ -818,7 +822,7 @@ describe('StoreStressConcurrent', () => { expect(print(store)).toEqual(snapshots[j]); // Stop forcing fallback. This reverts to primary content. - act(() => { + await actAsync(async () => { bridge.send('overrideSuspense', { id: suspenseID, rendererID: store.getRendererIDForElement(suspenseID), @@ -829,13 +833,13 @@ describe('StoreStressConcurrent', () => { expect(print(store)).toEqual(snapshots[i]); // Clean up after every iteration. - act(() => root.unmount()); + await actAsync(async () => root.unmount()); expect(print(store)).toBe(''); } } }); - it('should handle a stress test for Suspense without type change (Concurrent Mode)', () => { + it('should handle a stress test for Suspense without type change (Concurrent Mode)', async () => { const A = () => 'a'; const B = () => 'b'; const C = () => 'c'; @@ -1294,7 +1298,7 @@ describe('StoreStressConcurrent', () => { // Force fallback. expect(print(store)).toEqual(snapshots[i]); - act(() => { + await actAsync(async () => { bridge.send('overrideSuspense', { id: suspenseID, rendererID: store.getRendererIDForElement(suspenseID), @@ -1304,7 +1308,7 @@ describe('StoreStressConcurrent', () => { expect(print(store)).toEqual(fallbackSnapshots[j]); // Stop forcing fallback. - act(() => { + await actAsync(async () => { bridge.send('overrideSuspense', { id: suspenseID, rendererID: store.getRendererIDForElement(suspenseID), @@ -1354,7 +1358,7 @@ describe('StoreStressConcurrent', () => { expect(print(store)).toEqual(fallbackSnapshots[j]); // Stop forcing fallback. This reverts to primary content. - act(() => { + await actAsync(async () => { bridge.send('overrideSuspense', { id: suspenseID, rendererID: store.getRendererIDForElement(suspenseID), diff --git a/packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js b/packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js index 03bac9c657bd6..753af57ba849e 100644 --- a/packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js @@ -371,4 +371,54 @@ describe('ReactDOMNativeEventHeuristic-test', () => { ); } }); + + // @gate experimental + it('should not flush discrete events at the end of outermost batchedUpdates', async () => { + const root = ReactDOM.unstable_createRoot(container); + + let target; + function Foo() { + const [count, setCount] = React.useState(0); + return ( +
{ + target = el; + if (target !== null) { + el.onclick = () => { + ReactDOM.unstable_batchedUpdates(() => { + setCount(count + 1); + }); + Scheduler.unstable_yieldValue( + container.textContent + ' [after batchedUpdates]', + ); + }; + } + }}> + Count: {count} +
+ ); + } + + await act(async () => { + root.render(); + }); + expect(container.textContent).toEqual('Count: 0'); + + const pressEvent = document.createEvent('Event'); + pressEvent.initEvent('click', true, true); + dispatchAndSetCurrentEvent(target, pressEvent); + + expect(Scheduler).toHaveYielded(['Count: 0 [after batchedUpdates]']); + // TODO: There's a `flushDiscreteUpdates` call at the end of the event + // delegation listener that gets called even if no React event handlers are + // fired. Once that is removed, this will be 0, not 1. + // expect(container.textContent).toEqual('Count: 0'); + expect(container.textContent).toEqual('Count: 1'); + + // Intentionally not using `act` so we can observe in between the click + // event and the microtask, without batching. + await null; + + expect(container.textContent).toEqual('Count: 1'); + }); }); diff --git a/packages/react-reconciler/src/ReactFiberSyncTaskQueue.new.js b/packages/react-reconciler/src/ReactFiberSyncTaskQueue.new.js index 1d3e3daa47ae4..6518251603023 100644 --- a/packages/react-reconciler/src/ReactFiberSyncTaskQueue.new.js +++ b/packages/react-reconciler/src/ReactFiberSyncTaskQueue.new.js @@ -17,6 +17,7 @@ import { import {ImmediatePriority, scheduleCallback} from './Scheduler'; let syncQueue: Array | null = null; +let includesLegacySyncCallbacks: boolean = false; let isFlushingSyncQueue: boolean = false; export function scheduleSyncCallback(callback: SchedulerCallback) { @@ -31,7 +32,23 @@ export function scheduleSyncCallback(callback: SchedulerCallback) { } } -export function flushSyncCallbackQueue() { +export function scheduleLegacySyncCallback(callback: SchedulerCallback) { + includesLegacySyncCallbacks = true; + scheduleSyncCallback(callback); +} + +export function flushSyncCallbacksOnlyInLegacyMode() { + // Only flushes the queue if there's a legacy sync callback scheduled. + // TODO: There's only a single type of callback: performSyncOnWorkOnRoot. So + // it might make more sense for the queue to be a list of roots instead of a + // list of generic callbacks. Then we can have two: one for legacy roots, one + // for concurrent roots. And this method would only flush the legacy ones. + if (includesLegacySyncCallbacks) { + flushSyncCallbacks(); + } +} + +export function flushSyncCallbacks() { if (!isFlushingSyncQueue && syncQueue !== null) { // Prevent re-entrancy. isFlushingSyncQueue = true; @@ -50,13 +67,14 @@ export function flushSyncCallbackQueue() { } while (callback !== null); } syncQueue = null; + includesLegacySyncCallbacks = false; } catch (error) { // If something throws, leave the remaining callbacks on the queue. if (syncQueue !== null) { syncQueue = syncQueue.slice(i + 1); } // Resume flushing in the next tick - scheduleCallback(ImmediatePriority, flushSyncCallbackQueue); + scheduleCallback(ImmediatePriority, flushSyncCallbacks); throw error; } finally { setCurrentUpdatePriority(previousUpdatePriority); diff --git a/packages/react-reconciler/src/ReactFiberSyncTaskQueue.old.js b/packages/react-reconciler/src/ReactFiberSyncTaskQueue.old.js index 48a6f352fecab..52b4b5d44b640 100644 --- a/packages/react-reconciler/src/ReactFiberSyncTaskQueue.old.js +++ b/packages/react-reconciler/src/ReactFiberSyncTaskQueue.old.js @@ -17,6 +17,7 @@ import { import {ImmediatePriority, scheduleCallback} from './Scheduler'; let syncQueue: Array | null = null; +let includesLegacySyncCallbacks: boolean = false; let isFlushingSyncQueue: boolean = false; export function scheduleSyncCallback(callback: SchedulerCallback) { @@ -31,7 +32,23 @@ export function scheduleSyncCallback(callback: SchedulerCallback) { } } -export function flushSyncCallbackQueue() { +export function scheduleLegacySyncCallback(callback: SchedulerCallback) { + includesLegacySyncCallbacks = true; + scheduleSyncCallback(callback); +} + +export function flushSyncCallbacksOnlyInLegacyMode() { + // Only flushes the queue if there's a legacy sync callback scheduled. + // TODO: There's only a single type of callback: performSyncOnWorkOnRoot. So + // it might make more sense for the queue to be a list of roots instead of a + // list of generic callbacks. Then we can have two: one for legacy roots, one + // for concurrent roots. And this method would only flush the legacy ones. + if (includesLegacySyncCallbacks) { + flushSyncCallbacks(); + } +} + +export function flushSyncCallbacks() { if (!isFlushingSyncQueue && syncQueue !== null) { // Prevent re-entrancy. isFlushingSyncQueue = true; @@ -50,13 +67,14 @@ export function flushSyncCallbackQueue() { } while (callback !== null); } syncQueue = null; + includesLegacySyncCallbacks = false; } catch (error) { // If something throws, leave the remaining callbacks on the queue. if (syncQueue !== null) { syncQueue = syncQueue.slice(i + 1); } // Resume flushing in the next tick - scheduleCallback(ImmediatePriority, flushSyncCallbackQueue); + scheduleCallback(ImmediatePriority, flushSyncCallbacks); throw error; } finally { setCurrentUpdatePriority(previousUpdatePriority); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 9ebace0c72010..938d32fc826c6 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -49,8 +49,10 @@ import { IdlePriority as IdleSchedulerPriority, } from './Scheduler'; import { - flushSyncCallbackQueue, + flushSyncCallbacks, + flushSyncCallbacksOnlyInLegacyMode, scheduleSyncCallback, + scheduleLegacySyncCallback, } from './ReactFiberSyncTaskQueue.new'; import { NoFlags as NoHookEffect, @@ -561,7 +563,7 @@ export function scheduleUpdateOnFiber( // without immediately flushing it. We only do this for user-initiated // updates, to preserve historical behavior of legacy mode. resetRenderTimer(); - flushSyncCallbackQueue(); + flushSyncCallbacksOnlyInLegacyMode(); } } } else { @@ -698,13 +700,17 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) { if (newCallbackPriority === SyncLane) { // Special case: Sync React callbacks are scheduled on a special // internal queue - scheduleSyncCallback(performSyncWorkOnRoot.bind(null, root)); + if (root.tag === LegacyRoot) { + scheduleLegacySyncCallback(performSyncWorkOnRoot.bind(null, root)); + } else { + scheduleSyncCallback(performSyncWorkOnRoot.bind(null, root)); + } if (supportsMicrotasks) { // Flush the queue in a microtask. - scheduleMicrotask(flushSyncCallbackQueue); + scheduleMicrotask(flushSyncCallbacks); } else { // Flush the queue in an Immediate task. - scheduleCallback(ImmediateSchedulerPriority, flushSyncCallbackQueue); + scheduleCallback(ImmediateSchedulerPriority, flushSyncCallbacks); } newCallbackNode = null; } else { @@ -1054,7 +1060,7 @@ export function flushRoot(root: FiberRoot, lanes: Lanes) { ensureRootIsScheduled(root, now()); if ((executionContext & (RenderContext | CommitContext)) === NoContext) { resetRenderTimer(); - flushSyncCallbackQueue(); + flushSyncCallbacks(); } } } @@ -1085,7 +1091,7 @@ export function flushDiscreteUpdates() { // like `el.focus()`. Exit. return; } - flushSyncCallbackQueue(); + flushSyncCallbacks(); // If the discrete updates scheduled passive effects, flush them now so that // they fire before the next serial event. flushPassiveEffects(); @@ -1111,10 +1117,11 @@ export function batchedUpdates(fn: A => R, a: A): R { return fn(a); } finally { executionContext = prevExecutionContext; + // If there were legacy sync updates, flush them at the end of the outer + // most batchedUpdates-like method. if (executionContext === NoContext) { - // Flush the immediate callbacks that were scheduled during this batch resetRenderTimer(); - flushSyncCallbackQueue(); + flushSyncCallbacksOnlyInLegacyMode(); } } } @@ -1126,10 +1133,11 @@ export function batchedEventUpdates(fn: A => R, a: A): R { return fn(a); } finally { executionContext = prevExecutionContext; + // If there were legacy sync updates, flush them at the end of the outer + // most batchedUpdates-like method. if (executionContext === NoContext) { - // Flush the immediate callbacks that were scheduled during this batch resetRenderTimer(); - flushSyncCallbackQueue(); + flushSyncCallbacksOnlyInLegacyMode(); } } } @@ -1151,9 +1159,10 @@ export function discreteUpdates( setCurrentUpdatePriority(previousPriority); ReactCurrentBatchConfig.transition = prevTransition; if (executionContext === NoContext) { - // Flush the immediate callbacks that were scheduled during this batch resetRenderTimer(); - flushSyncCallbackQueue(); + // TODO: This should only flush legacy sync updates. Not discrete updates + // in Concurrent Mode. Discrete updates will flush in a microtask. + flushSyncCallbacks(); } } } @@ -1166,10 +1175,13 @@ export function unbatchedUpdates(fn: (a: A) => R, a: A): R { return fn(a); } finally { executionContext = prevExecutionContext; + // If there were legacy sync updates, flush them at the end of the outer + // most batchedUpdates-like method. if (executionContext === NoContext) { - // Flush the immediate callbacks that were scheduled during this batch resetRenderTimer(); - flushSyncCallbackQueue(); + // TODO: I think this call is redundant, because we flush inside + // scheduleUpdateOnFiber when LegacyUnbatchedContext is set. + flushSyncCallbacksOnlyInLegacyMode(); } } } @@ -1196,7 +1208,7 @@ export function flushSync(fn: A => R, a: A): R { // Note that this will happen even if batchedUpdates is higher up // the stack. if ((executionContext & (RenderContext | CommitContext)) === NoContext) { - flushSyncCallbackQueue(); + flushSyncCallbacks(); } else { if (__DEV__) { console.error( @@ -1226,7 +1238,7 @@ export function flushControlled(fn: () => mixed): void { if (executionContext === NoContext) { // Flush the immediate callbacks that were scheduled during this batch resetRenderTimer(); - flushSyncCallbackQueue(); + flushSyncCallbacks(); } } } @@ -2098,7 +2110,7 @@ function commitRootImpl(root, renderPriorityLevel) { } // If layout work was scheduled, flush it now. - flushSyncCallbackQueue(); + flushSyncCallbacks(); if (__DEV__) { if (enableDebugTracing) { @@ -2224,7 +2236,7 @@ function flushPassiveEffectsImpl() { executionContext = prevExecutionContext; - flushSyncCallbackQueue(); + flushSyncCallbacks(); // If additional passive effects were scheduled, increment a counter. If this // exceeds the limit, we'll fire a warning. diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 09a116c97bd7d..8f80ad2a37bc3 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -49,8 +49,10 @@ import { IdlePriority as IdleSchedulerPriority, } from './Scheduler'; import { - flushSyncCallbackQueue, + flushSyncCallbacks, + flushSyncCallbacksOnlyInLegacyMode, scheduleSyncCallback, + scheduleLegacySyncCallback, } from './ReactFiberSyncTaskQueue.old'; import { NoFlags as NoHookEffect, @@ -561,7 +563,7 @@ export function scheduleUpdateOnFiber( // without immediately flushing it. We only do this for user-initiated // updates, to preserve historical behavior of legacy mode. resetRenderTimer(); - flushSyncCallbackQueue(); + flushSyncCallbacksOnlyInLegacyMode(); } } } else { @@ -698,13 +700,17 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) { if (newCallbackPriority === SyncLane) { // Special case: Sync React callbacks are scheduled on a special // internal queue - scheduleSyncCallback(performSyncWorkOnRoot.bind(null, root)); + if (root.tag === LegacyRoot) { + scheduleLegacySyncCallback(performSyncWorkOnRoot.bind(null, root)); + } else { + scheduleSyncCallback(performSyncWorkOnRoot.bind(null, root)); + } if (supportsMicrotasks) { // Flush the queue in a microtask. - scheduleMicrotask(flushSyncCallbackQueue); + scheduleMicrotask(flushSyncCallbacks); } else { // Flush the queue in an Immediate task. - scheduleCallback(ImmediateSchedulerPriority, flushSyncCallbackQueue); + scheduleCallback(ImmediateSchedulerPriority, flushSyncCallbacks); } newCallbackNode = null; } else { @@ -1054,7 +1060,7 @@ export function flushRoot(root: FiberRoot, lanes: Lanes) { ensureRootIsScheduled(root, now()); if ((executionContext & (RenderContext | CommitContext)) === NoContext) { resetRenderTimer(); - flushSyncCallbackQueue(); + flushSyncCallbacks(); } } } @@ -1085,7 +1091,7 @@ export function flushDiscreteUpdates() { // like `el.focus()`. Exit. return; } - flushSyncCallbackQueue(); + flushSyncCallbacks(); // If the discrete updates scheduled passive effects, flush them now so that // they fire before the next serial event. flushPassiveEffects(); @@ -1111,10 +1117,11 @@ export function batchedUpdates(fn: A => R, a: A): R { return fn(a); } finally { executionContext = prevExecutionContext; + // If there were legacy sync updates, flush them at the end of the outer + // most batchedUpdates-like method. if (executionContext === NoContext) { - // Flush the immediate callbacks that were scheduled during this batch resetRenderTimer(); - flushSyncCallbackQueue(); + flushSyncCallbacksOnlyInLegacyMode(); } } } @@ -1126,10 +1133,11 @@ export function batchedEventUpdates(fn: A => R, a: A): R { return fn(a); } finally { executionContext = prevExecutionContext; + // If there were legacy sync updates, flush them at the end of the outer + // most batchedUpdates-like method. if (executionContext === NoContext) { - // Flush the immediate callbacks that were scheduled during this batch resetRenderTimer(); - flushSyncCallbackQueue(); + flushSyncCallbacksOnlyInLegacyMode(); } } } @@ -1151,9 +1159,10 @@ export function discreteUpdates( setCurrentUpdatePriority(previousPriority); ReactCurrentBatchConfig.transition = prevTransition; if (executionContext === NoContext) { - // Flush the immediate callbacks that were scheduled during this batch resetRenderTimer(); - flushSyncCallbackQueue(); + // TODO: This should only flush legacy sync updates. Not discrete updates + // in Concurrent Mode. Discrete updates will flush in a microtask. + flushSyncCallbacks(); } } } @@ -1166,10 +1175,13 @@ export function unbatchedUpdates(fn: (a: A) => R, a: A): R { return fn(a); } finally { executionContext = prevExecutionContext; + // If there were legacy sync updates, flush them at the end of the outer + // most batchedUpdates-like method. if (executionContext === NoContext) { - // Flush the immediate callbacks that were scheduled during this batch resetRenderTimer(); - flushSyncCallbackQueue(); + // TODO: I think this call is redundant, because we flush inside + // scheduleUpdateOnFiber when LegacyUnbatchedContext is set. + flushSyncCallbacksOnlyInLegacyMode(); } } } @@ -1196,7 +1208,7 @@ export function flushSync(fn: A => R, a: A): R { // Note that this will happen even if batchedUpdates is higher up // the stack. if ((executionContext & (RenderContext | CommitContext)) === NoContext) { - flushSyncCallbackQueue(); + flushSyncCallbacks(); } else { if (__DEV__) { console.error( @@ -1226,7 +1238,7 @@ export function flushControlled(fn: () => mixed): void { if (executionContext === NoContext) { // Flush the immediate callbacks that were scheduled during this batch resetRenderTimer(); - flushSyncCallbackQueue(); + flushSyncCallbacks(); } } } @@ -2098,7 +2110,7 @@ function commitRootImpl(root, renderPriorityLevel) { } // If layout work was scheduled, flush it now. - flushSyncCallbackQueue(); + flushSyncCallbacks(); if (__DEV__) { if (enableDebugTracing) { @@ -2224,7 +2236,7 @@ function flushPassiveEffectsImpl() { executionContext = prevExecutionContext; - flushSyncCallbackQueue(); + flushSyncCallbacks(); // If additional passive effects were scheduled, increment a counter. If this // exceeds the limit, we'll fire a warning. diff --git a/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.internal.js b/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.internal.js index 5eda4dc7c7cd0..a40a65671b8b2 100644 --- a/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.internal.js @@ -13,16 +13,20 @@ let React; let ReactFiberReconciler; let ConcurrentRoot; +let DefaultEventPriority; describe('ReactFiberHostContext', () => { beforeEach(() => { jest.resetModules(); React = require('react'); ReactFiberReconciler = require('react-reconciler'); - ConcurrentRoot = require('react-reconciler/src/ReactRootTags'); + ConcurrentRoot = require('react-reconciler/src/ReactRootTags') + .ConcurrentRoot; + DefaultEventPriority = require('react-reconciler/src/ReactEventPriorities') + .DefaultEventPriority; }); - it('works with null host context', () => { + it('works with null host context', async () => { let creates = 0; const Renderer = ReactFiberReconciler({ prepareForCommit: function() { @@ -54,6 +58,9 @@ describe('ReactFiberHostContext', () => { return null; }, clearContainer: function() {}, + getCurrentEventPriority: function() { + return DefaultEventPriority; + }, supportsMutation: true, }); @@ -63,14 +70,16 @@ describe('ReactFiberHostContext', () => { false, null, ); - Renderer.updateContainer( - - - , - container, - /* parentComponent: */ null, - /* callback: */ null, - ); + Renderer.act(() => { + Renderer.updateContainer( + + + , + container, + /* parentComponent: */ null, + /* callback: */ null, + ); + }); expect(creates).toBe(2); }); @@ -109,6 +118,9 @@ describe('ReactFiberHostContext', () => { return null; }, clearContainer: function() {}, + getCurrentEventPriority: function() { + return DefaultEventPriority; + }, supportsMutation: true, }); @@ -118,13 +130,15 @@ describe('ReactFiberHostContext', () => { false, null, ); - Renderer.updateContainer( - - - , - container, - /* parentComponent: */ null, - /* callback: */ null, - ); + Renderer.act(() => { + Renderer.updateContainer( + + + , + container, + /* parentComponent: */ null, + /* callback: */ null, + ); + }); }); }); diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalTriangle-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalTriangle-test.js index 275a3feb8c82f..f5dd5081aeaa8 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalTriangle-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalTriangle-test.js @@ -432,9 +432,8 @@ describe('ReactIncrementalTriangle', () => { assertConsistentTree(activeLeafIndices); } // Flush remaining work - ReactNoop.act(() => { - Scheduler.unstable_flushAllWithoutAsserting(); - }); + Scheduler.unstable_flushAllWithoutAsserting(); + ReactNoop.flushSync(); assertConsistentTree(activeLeafIndices, expectedCounterAtEnd); }