From 47ebc90b08be7a2e6955dd3cfd468318e0b8fdfd Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 6 May 2020 19:19:14 -0700 Subject: [PATCH] Put render phase update change behind a flag (#18850) In the new reconciler, I made a change to how render phase updates work. (By render phase updates, I mean when a component updates another component during its render phase. Or when a class component updates itself during the render phase. It does not include when a hook updates its own component during the render phase. Those have their own semantics. So really I mean anything triggers the "`setState` in render" warning.) The old behavior is to give the update the same "thread" (expiration time) as whatever is currently rendering. So if you call `setState` on a component that happens later in the same render, it will flush during that render. Ideally, we want to remove the special case and treat them as if they came from an interleaved event. Regardless, this pattern is not officially supported. This behavior is only a fallback. The flag only exists until we can roll out the `setState` warnning, since existing code might accidentally rely on the current behavior. --- .../ReactDOMServerIntegrationHooks-test.js | 8 +++-- .../src/ReactFiberWorkLoop.new.js | 35 ++++++++++++++++--- .../__tests__/ReactIncrementalUpdates-test.js | 4 +-- packages/shared/ReactFeatureFlags.js | 8 +++++ .../forks/ReactFeatureFlags.native-fb.js | 1 + .../forks/ReactFeatureFlags.native-oss.js | 1 + .../forks/ReactFeatureFlags.test-renderer.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 | 10 ++++++ .../shared/forks/ReactFeatureFlags.www.js | 1 + 12 files changed, 63 insertions(+), 9 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js index 8c90d593a7..79345cc146 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js @@ -1686,7 +1686,9 @@ describe('ReactDOMServerHooks', () => { , ); - if (gate(flags => flags.new)) { + if ( + gate(flags => flags.new && flags.deferRenderPhaseUpdateToNextBatch) + ) { expect(() => Scheduler.unstable_flushAll()).toErrorDev([ 'The object passed back from useOpaqueIdentifier is meant to be passed through to attributes only. ' + 'Do not read the value directly.', @@ -1730,7 +1732,9 @@ describe('ReactDOMServerHooks', () => { , ); - if (gate(flags => flags.new)) { + if ( + gate(flags => flags.new && flags.deferRenderPhaseUpdateToNextBatch) + ) { expect(() => Scheduler.unstable_flushAll()).toErrorDev([ 'The object passed back from useOpaqueIdentifier is meant to be passed through to attributes only. ' + 'Do not read the value directly.', diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index e2ff1032b8..3c6b2105d0 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -27,6 +27,7 @@ import { enableProfilerCommitHooks, enableSchedulerTracing, warnAboutUnmockedScheduler, + deferRenderPhaseUpdateToNextBatch, } from 'shared/ReactFeatureFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import invariant from 'shared/invariant'; @@ -123,6 +124,7 @@ import { isSubsetOfLanes, mergeLanes, removeLanes, + pickArbitraryLane, hasDiscreteLanes, hasUpdatePriority, getNextLanes, @@ -354,6 +356,21 @@ export function requestUpdateLane( return getCurrentPriorityLevel() === ImmediateSchedulerPriority ? (SyncLane: Lane) : (SyncBatchedLane: Lane); + } else if ( + !deferRenderPhaseUpdateToNextBatch && + (executionContext & RenderContext) !== NoContext && + workInProgressRootRenderLanes !== NoLanes + ) { + // This is a render phase update. These are not officially supported. The + // old behavior is to give this the same "thread" (expiration time) as + // whatever is currently rendering. So if you call `setState` on a component + // that happens later in the same render, it will flush. Ideally, we want to + // remove the special case and treat them as if they came from an + // interleaved event. Regardless, this pattern is not officially supported. + // This behavior is only a fallback. The flag only exists until we can roll + // out the setState warnning, since existing code might accidentally rely on + // the current behavior. + return pickArbitraryLane(workInProgressRootRenderLanes); } // The algorithm for assigning an update to a lane should be stable for all @@ -542,11 +559,19 @@ function markUpdateLaneFromFiberToRoot( markRootUpdated(root, lane); if (workInProgressRoot === root) { // Received an update to a tree that's in the middle of rendering. Mark - // that there is unprocessed work on this root. - workInProgressRootUpdatedLanes = mergeLanes( - workInProgressRootUpdatedLanes, - lane, - ); + // that there was an interleaved update work on this root. Unless the + // `deferRenderPhaseUpdateToNextBatch` flag is off and this is a render + // phase update. In that case, we don't treat render phase updates as if + // they were interleaved, for backwards compat reasons. + if ( + deferRenderPhaseUpdateToNextBatch || + (executionContext & RenderContext) === NoContext + ) { + workInProgressRootUpdatedLanes = mergeLanes( + workInProgressRootUpdatedLanes, + lane, + ); + } if (workInProgressRootExitStatus === RootSuspendedWithDelay) { // The root already suspended with a delay, which means this render // definitely won't finish. Since we have a new update, let's mark it as diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js index 90a327d734..055a2ee5c6 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js @@ -394,7 +394,7 @@ describe('ReactIncrementalUpdates', () => { expect(() => expect(Scheduler).toFlushAndYield( gate(flags => - flags.new + flags.new && flags.deferRenderPhaseUpdateToNextBatch ? [ 'setState updater', // In the new reconciler, updates inside the render phase are @@ -427,7 +427,7 @@ describe('ReactIncrementalUpdates', () => { }); expect(Scheduler).toFlushAndYield( gate(flags => - flags.new + flags.new && flags.deferRenderPhaseUpdateToNextBatch ? // In the new reconciler, updates inside the render phase are // treated as if they came from an event, so the update gets shifted // to a subsequent render. diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 314b9a2447..b0e85d43d3 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -127,3 +127,11 @@ export const enableModernEventSystem = false; // Support legacy Primer support on internal FB www export const enableLegacyFBSupport = false; + +// Updates that occur in the render phase are not officially supported. But when +// they do occur, in the new reconciler, we defer them to a subsequent render by +// picking a lane that's not currently rendering. We treat them the same as if +// they came from an interleaved event. In the old reconciler, we use whatever +// expiration time is currently rendering. Remove this flag once we have +// migrated to the new behavior. +export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 1d4198c3fb..978c5ff8cd 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -46,6 +46,7 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const enableNewReconciler = false; +export const deferRenderPhaseUpdateToNextBatch = true; // 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 6ba9fa00cf..9c00c127f2 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -45,6 +45,7 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const enableNewReconciler = false; +export const deferRenderPhaseUpdateToNextBatch = true; // 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 21ed6623e1..93ff1a7fa0 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -45,6 +45,7 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const enableNewReconciler = false; +export const deferRenderPhaseUpdateToNextBatch = true; // 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 5cd46d0cff..c84df882fb 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -45,6 +45,7 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const enableNewReconciler = false; +export const deferRenderPhaseUpdateToNextBatch = true; // 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 41971ca576..2aa405ebe6 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.js @@ -45,6 +45,7 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const enableNewReconciler = false; +export const deferRenderPhaseUpdateToNextBatch = true; // 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 12c09a97e6..3c7ae77f8e 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.www.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.www.js @@ -45,6 +45,7 @@ export const enableLegacyFBSupport = !__EXPERIMENTAL__; export const enableFilterEmptyStringAttributesDOM = false; export const enableNewReconciler = false; +export const deferRenderPhaseUpdateToNextBatch = true; // 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 766b4ebe7d..a047ec5d72 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -21,6 +21,16 @@ export const enableModernEventSystem = __VARIANT__; export const enableLegacyFBSupport = __VARIANT__; export const enableDebugTracing = !__VARIANT__; +// This only has an effect in the new reconciler. But also, the new reconciler +// is only enabled when __VARIANT__ is true. So this is set to the opposite of +// __VARIANT__ so that it's `false` when running against the new reconciler. +// Ideally we would test both against the new reconciler, but until then, we +// should test the value that is used in www. Which is `false`. +// +// Once Lanes has landed in both reconciler forks, we'll get coverage of +// both branches. +export const deferRenderPhaseUpdateToNextBatch = !__VARIANT__; + // These are already tested in both modes using the build type dimension, // so we don't need to use __VARIANT__ to get extra coverage. export const debugRenderPhaseSideEffectsForStrictMode = __DEV__; diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index d21c7b7b69..cc982e8cd5 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -26,6 +26,7 @@ export const { enableFilterEmptyStringAttributesDOM, enableLegacyFBSupport, enableDebugTracing, + deferRenderPhaseUpdateToNextBatch, } = dynamicFeatureFlags; // On WWW, __EXPERIMENTAL__ is used for a new modern build.