Put render phase update change behind a flag (facebook#18850)

acdlite · web-flow · commit 47ebc90b08be · 2020-05-06T19:19:14.000-07:00
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.
diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js
@@ -1686,7 +1686,9 @@ describe('ReactDOMServerHooks', () => {
           <App />,
         );
 
-        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', () => {
           <App />,
         );
 
-        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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -26,6 +26,7 @@ export const {
   enableFilterEmptyStringAttributesDOM,
   enableLegacyFBSupport,
   enableDebugTracing,
+  deferRenderPhaseUpdateToNextBatch,
 } = dynamicFeatureFlags;
 
 // On WWW, __EXPERIMENTAL__ is used for a new modern build.
