From e51bd6c1fa2731c4fcd39300144e917aedfc989b Mon Sep 17 00:00:00 2001 From: Ricky Date: Wed, 27 Jan 2021 18:24:58 -0500 Subject: [PATCH] Queue discrete events in microtask (#20669) * Queue discrete events in microtask * Use callback priority to determine cancellation * Add queueMicrotask to react-reconciler README * Fix invatiant conditon for InputDiscrete * Switch invariant null check * Convert invariant to warning * Remove warning from codes.json --- .../src/client/ReactDOMHostConfig.js | 2 +- .../__tests__/ChangeEventPlugin-test.js | 11 ++++- .../__tests__/SimpleEventPlugin-test.js | 23 +++++++--- packages/react-reconciler/README.md | 4 ++ .../src/ReactFiberWorkLoop.new.js | 42 +++++++++++++++---- .../src/ReactFiberWorkLoop.old.js | 42 +++++++++++++++---- .../ReactSuspenseWithNoopRenderer-test.js | 1 + .../src/ReactTestHostConfig.js | 2 +- packages/shared/ReactFeatureFlags.js | 2 + .../forks/ReactFeatureFlags.native-fb.js | 1 + .../forks/ReactFeatureFlags.native-oss.js | 1 + .../forks/ReactFeatureFlags.test-renderer.js | 1 + .../ReactFeatureFlags.test-renderer.native.js | 1 + .../ReactFeatureFlags.test-renderer.www.js | 1 + .../shared/forks/ReactFeatureFlags.testing.js | 1 + .../forks/ReactFeatureFlags.testing.www.js | 1 + .../forks/ReactFeatureFlags.www-dynamic.js | 1 + .../shared/forks/ReactFeatureFlags.www.js | 1 + 18 files changed, 111 insertions(+), 27 deletions(-) diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index 969f6f88b02a6..fedfe8698e1ff 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -392,7 +392,7 @@ export const queueMicrotask: any = Promise.resolve(null) .then(callback) .catch(handleErrorInNextTick) - : scheduleTimeout; + : scheduleTimeout; // TODO: Determine the best fallback here. function handleErrorInNextTick(error) { setTimeout(() => { diff --git a/packages/react-dom/src/events/plugins/__tests__/ChangeEventPlugin-test.js b/packages/react-dom/src/events/plugins/__tests__/ChangeEventPlugin-test.js index 2a806b2888c18..45828e2735ed7 100644 --- a/packages/react-dom/src/events/plugins/__tests__/ChangeEventPlugin-test.js +++ b/packages/react-dom/src/events/plugins/__tests__/ChangeEventPlugin-test.js @@ -730,8 +730,15 @@ describe('ChangeEventPlugin', () => { // Flush callbacks. // Now the click update has flushed. - expect(Scheduler).toFlushAndYield(['render: ']); - expect(input.value).toBe(''); + if (gate(flags => flags.enableDiscreteEventMicroTasks)) { + // Flush microtask queue. + await null; + expect(Scheduler).toHaveYielded(['render: ']); + expect(input.value).toBe(''); + } else { + expect(Scheduler).toFlushAndYield(['render: ']); + expect(input.value).toBe(''); + } }); // @gate experimental diff --git a/packages/react-dom/src/events/plugins/__tests__/SimpleEventPlugin-test.js b/packages/react-dom/src/events/plugins/__tests__/SimpleEventPlugin-test.js index 0579e9571cbc2..377fa61e507d7 100644 --- a/packages/react-dom/src/events/plugins/__tests__/SimpleEventPlugin-test.js +++ b/packages/react-dom/src/events/plugins/__tests__/SimpleEventPlugin-test.js @@ -470,11 +470,24 @@ describe('SimpleEventPlugin', function() { 'High-pri count: 7, Low-pri count: 0', ]); - // At the end, both counters should equal the total number of clicks - expect(Scheduler).toFlushAndYield([ - 'High-pri count: 8, Low-pri count: 0', - 'High-pri count: 8, Low-pri count: 8', - ]); + if (gate(flags => flags.enableDiscreteEventMicroTasks)) { + // Flush the microtask queue + await null; + + // At the end, both counters should equal the total number of clicks + expect(Scheduler).toHaveYielded([ + 'High-pri count: 8, Low-pri count: 0', + + // TODO: with cancellation, this required another flush? + 'High-pri count: 8, Low-pri count: 8', + ]); + } else { + // At the end, both counters should equal the total number of clicks + expect(Scheduler).toFlushAndYield([ + 'High-pri count: 8, Low-pri count: 0', + 'High-pri count: 8, Low-pri count: 8', + ]); + } expect(button.textContent).toEqual('High-pri count: 8, Low-pri count: 8'); }); }); diff --git a/packages/react-reconciler/README.md b/packages/react-reconciler/README.md index 5fb890f381656..217d5f4b8e551 100644 --- a/packages/react-reconciler/README.md +++ b/packages/react-reconciler/README.md @@ -203,6 +203,10 @@ You can proxy this to `clearTimeout` or its equivalent in your environment. This is a property (not a function) that should be set to something that can never be a valid timeout ID. For example, you can set it to `-1`. +#### `queueMicrotask(fn)` + +You can proxy this to `queueMicrotask` or its equivalent in your environment. + #### `isPrimaryRenderer` This is a property (not a function) that should be set to `true` if your renderer is the main one on the page. For example, if you're writing a renderer for the Terminal, it makes sense to set it to `true`, but if your renderer is used *on top of* React DOM or some other existing renderer, set it to `false`. diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 59fce3d254f0b..23d8037b4ff25 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -92,6 +92,7 @@ import { warnsIfNotActing, afterActiveInstanceBlur, clearContainer, + queueMicrotask, } from './ReactFiberHostConfig'; import { @@ -216,6 +217,7 @@ import { syncNestedUpdateFlag, } from './ReactProfilerTimer.new'; +import {enableDiscreteEventMicroTasks} from 'shared/ReactFeatureFlags'; // DEV stuff import getComponentName from 'shared/getComponentName'; import ReactStrictModeWarnings from './ReactStrictModeWarnings.new'; @@ -714,21 +716,34 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) { // Special case: There's nothing to work on. if (existingCallbackNode !== null) { cancelCallback(existingCallbackNode); - root.callbackNode = null; - root.callbackPriority = NoLanePriority; } + root.callbackNode = null; + root.callbackPriority = NoLanePriority; return; } // Check if there's an existing task. We may be able to reuse it. - if (existingCallbackNode !== null) { - const existingCallbackPriority = root.callbackPriority; - if (existingCallbackPriority === newCallbackPriority) { - // The priority hasn't changed. We can reuse the existing task. Exit. - return; + const existingCallbackPriority = root.callbackPriority; + if (existingCallbackPriority === newCallbackPriority) { + if (__DEV__) { + // If we're going to re-use an existing task, it needs to exist. + // Assume that discrete update microtasks are non-cancellable and null. + // TODO: Temporary until we confirm this warning is not fired. + if ( + existingCallbackNode == null && + existingCallbackPriority !== InputDiscreteLanePriority + ) { + console.error( + 'Expected scheduled callback to exist. This error is likely caused by a bug in React. Please file an issue.', + ); + } } - // The priority changed. Cancel the existing callback. We'll schedule a new - // one below. + // The priority hasn't changed. We can reuse the existing task. Exit. + return; + } + + if (existingCallbackNode != null) { + // Cancel the existing callback. We'll schedule a new one below. cancelCallback(existingCallbackNode); } @@ -737,6 +752,8 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) { if (newCallbackPriority === SyncLanePriority) { // Special case: Sync React callbacks are scheduled on a special // internal queue + + // TODO: After enableDiscreteEventMicroTasks lands, we can remove the fake node. newCallbackNode = scheduleSyncCallback( performSyncWorkOnRoot.bind(null, root), ); @@ -745,6 +762,12 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) { ImmediateSchedulerPriority, performSyncWorkOnRoot.bind(null, root), ); + } else if ( + enableDiscreteEventMicroTasks && + newCallbackPriority === InputDiscreteLanePriority + ) { + queueMicrotask(performSyncWorkOnRoot.bind(null, root)); + newCallbackNode = null; } else { const schedulerPriorityLevel = lanePriorityToSchedulerPriority( newCallbackPriority, @@ -1871,6 +1894,7 @@ function commitRootImpl(root, renderPriorityLevel) { // commitRoot never returns a continuation; it always finishes synchronously. // So we can clear these now to allow a new callback to be scheduled. root.callbackNode = null; + root.callbackPriority = NoLanePriority; // Update the first and last pending times on this root. The new first // pending time is whatever is left on the root fiber. diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 833fa0e98b4fc..e1431de6431fd 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -34,6 +34,7 @@ import { disableSchedulerTimeoutInWorkLoop, enableDoubleInvokingEffects, skipUnmountedBoundaries, + enableDiscreteEventMicroTasks, } from 'shared/ReactFeatureFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import invariant from 'shared/invariant'; @@ -92,6 +93,7 @@ import { warnsIfNotActing, afterActiveInstanceBlur, clearContainer, + queueMicrotask, } from './ReactFiberHostConfig'; import { @@ -696,21 +698,34 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) { // Special case: There's nothing to work on. if (existingCallbackNode !== null) { cancelCallback(existingCallbackNode); - root.callbackNode = null; - root.callbackPriority = NoLanePriority; } + root.callbackNode = null; + root.callbackPriority = NoLanePriority; return; } // Check if there's an existing task. We may be able to reuse it. - if (existingCallbackNode !== null) { - const existingCallbackPriority = root.callbackPriority; - if (existingCallbackPriority === newCallbackPriority) { - // The priority hasn't changed. We can reuse the existing task. Exit. - return; + const existingCallbackPriority = root.callbackPriority; + if (existingCallbackPriority === newCallbackPriority) { + if (__DEV__) { + // If we're going to re-use an existing task, it needs to exist. + // Assume that discrete update microtasks are non-cancellable and null. + // TODO: Temporary until we confirm this warning is not fired. + if ( + existingCallbackNode == null && + existingCallbackPriority !== InputDiscreteLanePriority + ) { + console.error( + 'Expected scheduled callback to exist. This error is likely caused by a bug in React. Please file an issue.', + ); + } } - // The priority changed. Cancel the existing callback. We'll schedule a new - // one below. + // The priority hasn't changed. We can reuse the existing task. Exit. + return; + } + + if (existingCallbackNode != null) { + // Cancel the existing callback. We'll schedule a new one below. cancelCallback(existingCallbackNode); } @@ -719,6 +734,8 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) { if (newCallbackPriority === SyncLanePriority) { // Special case: Sync React callbacks are scheduled on a special // internal queue + + // TODO: After enableDiscreteEventMicroTasks lands, we can remove the fake node. newCallbackNode = scheduleSyncCallback( performSyncWorkOnRoot.bind(null, root), ); @@ -727,6 +744,12 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) { ImmediateSchedulerPriority, performSyncWorkOnRoot.bind(null, root), ); + } else if ( + enableDiscreteEventMicroTasks && + newCallbackPriority === InputDiscreteLanePriority + ) { + queueMicrotask(performSyncWorkOnRoot.bind(null, root)); + newCallbackNode = null; } else { const schedulerPriorityLevel = lanePriorityToSchedulerPriority( newCallbackPriority, @@ -1851,6 +1874,7 @@ function commitRootImpl(root, renderPriorityLevel) { // commitRoot never returns a continuation; it always finishes synchronously. // So we can clear these now to allow a new callback to be scheduled. root.callbackNode = null; + root.callbackPriority = NoLanePriority; // Update the first and last pending times on this root. The new first // pending time is whatever is left on the root fiber. diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js index 5021c32c8b11e..5a9c94d3a3acc 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js @@ -3528,6 +3528,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); // @gate enableCache + // @gate !enableDiscreteEventMicroTasks it('regression: empty render at high priority causes update to be dropped', async () => { // Reproduces a bug where flushDiscreteUpdates starts a new (empty) render // pass which cancels a scheduled timeout and causes the fallback never to diff --git a/packages/react-test-renderer/src/ReactTestHostConfig.js b/packages/react-test-renderer/src/ReactTestHostConfig.js index 026c74bd96657..c9f9b2376ebb1 100644 --- a/packages/react-test-renderer/src/ReactTestHostConfig.js +++ b/packages/react-test-renderer/src/ReactTestHostConfig.js @@ -228,7 +228,7 @@ export const queueMicrotask = Promise.resolve(null) .then(callback) .catch(handleErrorInNextTick) - : scheduleTimeout; + : scheduleTimeout; // TODO: Determine the best fallback here. function handleErrorInNextTick(error) { setTimeout(() => { diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index e09482a525ac3..53f9b744cb9c3 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -152,3 +152,5 @@ export const disableSchedulerTimeoutInWorkLoop = false; // Experiment to simplify/improve how transitions are scheduled export const enableTransitionEntanglement = false; + +export const enableDiscreteEventMicroTasks = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 494cae50c8b4d..a230930cb0a6a 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -59,6 +59,7 @@ export const enableUseRefAccessWarning = false; export const enableRecursiveCommitTraversal = false; export const disableSchedulerTimeoutInWorkLoop = false; export const enableTransitionEntanglement = false; +export const enableDiscreteEventMicroTasks = false; // Flow magic to verify the exports of this file match the original version. // eslint-disable-next-line no-unused-vars diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index 6c4616798eb37..0dc9bed575843 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -58,6 +58,7 @@ export const enableUseRefAccessWarning = false; export const enableRecursiveCommitTraversal = false; export const disableSchedulerTimeoutInWorkLoop = false; export const enableTransitionEntanglement = false; +export const enableDiscreteEventMicroTasks = false; // Flow magic to verify the exports of this file match the original version. // eslint-disable-next-line no-unused-vars diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 3971598060d52..5652b66a0b835 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -58,6 +58,7 @@ export const enableUseRefAccessWarning = false; export const enableRecursiveCommitTraversal = false; export const disableSchedulerTimeoutInWorkLoop = false; export const enableTransitionEntanglement = false; +export const enableDiscreteEventMicroTasks = false; // Flow magic to verify the exports of this file match the original version. // eslint-disable-next-line no-unused-vars diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js index 0d31d83b657f0..8962bcce36a41 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -58,6 +58,7 @@ export const enableUseRefAccessWarning = false; export const enableRecursiveCommitTraversal = false; export const disableSchedulerTimeoutInWorkLoop = false; export const enableTransitionEntanglement = false; +export const enableDiscreteEventMicroTasks = false; // Flow magic to verify the exports of this file match the original version. // eslint-disable-next-line no-unused-vars diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index f0479741a570e..7905485bce686 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -58,6 +58,7 @@ export const enableUseRefAccessWarning = false; export const enableRecursiveCommitTraversal = false; export const disableSchedulerTimeoutInWorkLoop = false; export const enableTransitionEntanglement = false; +export const enableDiscreteEventMicroTasks = false; // Flow magic to verify the exports of this file match the original version. // eslint-disable-next-line no-unused-vars diff --git a/packages/shared/forks/ReactFeatureFlags.testing.js b/packages/shared/forks/ReactFeatureFlags.testing.js index 3c2da8f5fc9d9..c1a20dd91a324 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.js @@ -58,6 +58,7 @@ export const enableUseRefAccessWarning = false; export const enableRecursiveCommitTraversal = false; export const disableSchedulerTimeoutInWorkLoop = false; export const enableTransitionEntanglement = false; +export const enableDiscreteEventMicroTasks = false; // Flow magic to verify the exports of this file match the original version. // eslint-disable-next-line no-unused-vars diff --git a/packages/shared/forks/ReactFeatureFlags.testing.www.js b/packages/shared/forks/ReactFeatureFlags.testing.www.js index 26eda01b2a244..c274d6a376450 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.www.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.www.js @@ -58,6 +58,7 @@ export const enableUseRefAccessWarning = false; export const enableRecursiveCommitTraversal = false; export const disableSchedulerTimeoutInWorkLoop = false; export const enableTransitionEntanglement = false; +export const enableDiscreteEventMicroTasks = false; // Flow magic to verify the exports of this file match the original version. // eslint-disable-next-line no-unused-vars diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js index e6cbe5c387f5d..47d8763d039b2 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -56,3 +56,4 @@ export const enableUseRefAccessWarning = __VARIANT__; export const enableProfilerNestedUpdateScheduledHook = __VARIANT__; export const disableSchedulerTimeoutInWorkLoop = __VARIANT__; export const enableTransitionEntanglement = __VARIANT__; +export const enableDiscreteEventMicroTasks = __VARIANT__; diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index f39c8bee578e2..2b1ba37ee8486 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -32,6 +32,7 @@ export const { disableNativeComponentFrames, disableSchedulerTimeoutInWorkLoop, enableTransitionEntanglement, + enableDiscreteEventMicroTasks, } = dynamicFeatureFlags; // On WWW, __EXPERIMENTAL__ is used for a new modern build.