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.
Andrew Clark committed
May 6, 2020 at 19:19 UTC
47ebc90b08be7a2e6955dd3cfd468318e0b8fdfd
12 files changed
+63
-9
packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js
+6
-2
@@ -1686,7 +1686,9 @@ describe('ReactDOMServerHooks', () => {
1686
<App />,
1687
);
1688
1689
- if (gate(flags => flags.new)) {
1689
+ if (
1690
+ gate(flags => flags.new && flags.deferRenderPhaseUpdateToNextBatch)
1691
+ ) {
1692
expect(() => Scheduler.unstable_flushAll()).toErrorDev([
1693
'The object passed back from useOpaqueIdentifier is meant to be passed through to attributes only. ' +
1694
'Do not read the value directly.',
@@ -1730,7 +1732,9 @@ describe('ReactDOMServerHooks', () => {
1732
<App />,
1733
);
1734
1733
- if (gate(flags => flags.new)) {
1735
+ if (
1736
+ gate(flags => flags.new && flags.deferRenderPhaseUpdateToNextBatch)
1737
+ ) {
1738
expect(() => Scheduler.unstable_flushAll()).toErrorDev([
1739
'The object passed back from useOpaqueIdentifier is meant to be passed through to attributes only. ' +
1740
'Do not read the value directly.',
packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+30
-5
@@ -27,6 +27,7 @@ import {
27
enableProfilerCommitHooks,
28
enableSchedulerTracing,
29
warnAboutUnmockedScheduler,
30
+ deferRenderPhaseUpdateToNextBatch,
31
} from 'shared/ReactFeatureFlags';
32
import ReactSharedInternals from 'shared/ReactSharedInternals';
33
import invariant from 'shared/invariant';
@@ -123,6 +124,7 @@ import {
124
isSubsetOfLanes,
125
mergeLanes,
126
removeLanes,
127
+ pickArbitraryLane,
128
hasDiscreteLanes,
129
hasUpdatePriority,
130
getNextLanes,
@@ -354,6 +356,21 @@ export function requestUpdateLane(
356
return getCurrentPriorityLevel() === ImmediateSchedulerPriority
357
? (SyncLane: Lane)
358
: (SyncBatchedLane: Lane);
359
+ } else if (
360
+ !deferRenderPhaseUpdateToNextBatch &&
361
+ (executionContext & RenderContext) !== NoContext &&
362
+ workInProgressRootRenderLanes !== NoLanes
363
+ ) {
364
+ // This is a render phase update. These are not officially supported. The
365
+ // old behavior is to give this the same "thread" (expiration time) as
366
+ // whatever is currently rendering. So if you call `setState` on a component
367
+ // that happens later in the same render, it will flush. Ideally, we want to
368
+ // remove the special case and treat them as if they came from an
369
+ // interleaved event. Regardless, this pattern is not officially supported.
370
+ // This behavior is only a fallback. The flag only exists until we can roll
371
+ // out the setState warnning, since existing code might accidentally rely on
372
+ // the current behavior.
373
+ return pickArbitraryLane(workInProgressRootRenderLanes);
374
}
375
376
// The algorithm for assigning an update to a lane should be stable for all
@@ -542,11 +559,19 @@ function markUpdateLaneFromFiberToRoot(
559
markRootUpdated(root, lane);
560
if (workInProgressRoot === root) {
561
// Received an update to a tree that's in the middle of rendering. Mark
545
- // that there is unprocessed work on this root.
546
- workInProgressRootUpdatedLanes = mergeLanes(
547
- workInProgressRootUpdatedLanes,
548
- lane,
549
- );
562
+ // that there was an interleaved update work on this root. Unless the
563
+ // `deferRenderPhaseUpdateToNextBatch` flag is off and this is a render
564
+ // phase update. In that case, we don't treat render phase updates as if
565
+ // they were interleaved, for backwards compat reasons.
566
+ if (
567
+ deferRenderPhaseUpdateToNextBatch ||
568
+ (executionContext & RenderContext) === NoContext
569
+ ) {
570
+ workInProgressRootUpdatedLanes = mergeLanes(
571
+ workInProgressRootUpdatedLanes,
572
+ lane,
573
+ );
574
+ }
575
if (workInProgressRootExitStatus === RootSuspendedWithDelay) {
576
// The root already suspended with a delay, which means this render
577
// definitely won't finish. Since we have a new update, let's mark it as
packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js
+2
-2
@@ -394,7 +394,7 @@ describe('ReactIncrementalUpdates', () => {
394
expect(() =>
395
expect(Scheduler).toFlushAndYield(
396
gate(flags =>
397
- flags.new
397
+ flags.new && flags.deferRenderPhaseUpdateToNextBatch
398
? [
399
'setState updater',
400
// In the new reconciler, updates inside the render phase are
@@ -427,7 +427,7 @@ describe('ReactIncrementalUpdates', () => {
427
});
428
expect(Scheduler).toFlushAndYield(
429
gate(flags =>
430
- flags.new
430
+ flags.new && flags.deferRenderPhaseUpdateToNextBatch
431
? // In the new reconciler, updates inside the render phase are
432
// treated as if they came from an event, so the update gets shifted
433
// to a subsequent render.
packages/shared/ReactFeatureFlags.js
+8
@@ -127,3 +127,11 @@ export const enableModernEventSystem = false;
127
128
// Support legacy Primer support on internal FB www
129
export const enableLegacyFBSupport = false;
130
+
131
+// Updates that occur in the render phase are not officially supported. But when
132
+// they do occur, in the new reconciler, we defer them to a subsequent render by
133
+// picking a lane that's not currently rendering. We treat them the same as if
134
+// they came from an interleaved event. In the old reconciler, we use whatever
135
+// expiration time is currently rendering. Remove this flag once we have
136
+// migrated to the new behavior.
137
+export const deferRenderPhaseUpdateToNextBatch = true;
packages/shared/forks/ReactFeatureFlags.native-fb.js
+1
@@ -46,6 +46,7 @@ export const enableLegacyFBSupport = false;
46
export const enableFilterEmptyStringAttributesDOM = false;
47
48
export const enableNewReconciler = false;
49
+export const deferRenderPhaseUpdateToNextBatch = true;
50
51
// Flow magic to verify the exports of this file match the original version.
52
// eslint-disable-next-line no-unused-vars
packages/shared/forks/ReactFeatureFlags.native-oss.js
+1
@@ -45,6 +45,7 @@ export const enableLegacyFBSupport = false;
45
export const enableFilterEmptyStringAttributesDOM = false;
46
47
export const enableNewReconciler = false;
48
+export const deferRenderPhaseUpdateToNextBatch = true;
49
50
// Flow magic to verify the exports of this file match the original version.
51
// eslint-disable-next-line no-unused-vars
packages/shared/forks/ReactFeatureFlags.test-renderer.js
+1
@@ -45,6 +45,7 @@ export const enableLegacyFBSupport = false;
45
export const enableFilterEmptyStringAttributesDOM = false;
46
47
export const enableNewReconciler = false;
48
+export const deferRenderPhaseUpdateToNextBatch = true;
49
50
// Flow magic to verify the exports of this file match the original version.
51
// eslint-disable-next-line no-unused-vars
packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
+1
@@ -45,6 +45,7 @@ export const enableLegacyFBSupport = false;
45
export const enableFilterEmptyStringAttributesDOM = false;
46
47
export const enableNewReconciler = false;
48
+export const deferRenderPhaseUpdateToNextBatch = true;
49
50
// Flow magic to verify the exports of this file match the original version.
51
// eslint-disable-next-line no-unused-vars
packages/shared/forks/ReactFeatureFlags.testing.js
+1
@@ -45,6 +45,7 @@ export const enableLegacyFBSupport = false;
45
export const enableFilterEmptyStringAttributesDOM = false;
46
47
export const enableNewReconciler = false;
48
+export const deferRenderPhaseUpdateToNextBatch = true;
49
50
// Flow magic to verify the exports of this file match the original version.
51
// eslint-disable-next-line no-unused-vars
packages/shared/forks/ReactFeatureFlags.testing.www.js
+1
@@ -45,6 +45,7 @@ export const enableLegacyFBSupport = !__EXPERIMENTAL__;
45
export const enableFilterEmptyStringAttributesDOM = false;
46
47
export const enableNewReconciler = false;
48
+export const deferRenderPhaseUpdateToNextBatch = true;
49
50
// Flow magic to verify the exports of this file match the original version.
51
// eslint-disable-next-line no-unused-vars
packages/shared/forks/ReactFeatureFlags.www-dynamic.js
+10
@@ -21,6 +21,16 @@ export const enableModernEventSystem = __VARIANT__;
21
export const enableLegacyFBSupport = __VARIANT__;
22
export const enableDebugTracing = !__VARIANT__;
23
24
+// This only has an effect in the new reconciler. But also, the new reconciler
25
+// is only enabled when __VARIANT__ is true. So this is set to the opposite of
26
+// __VARIANT__ so that it's `false` when running against the new reconciler.
27
+// Ideally we would test both against the new reconciler, but until then, we
28
+// should test the value that is used in www. Which is `false`.
29
+//
30
+// Once Lanes has landed in both reconciler forks, we'll get coverage of
31
+// both branches.
32
+export const deferRenderPhaseUpdateToNextBatch = !__VARIANT__;
33
+
34
// These are already tested in both modes using the build type dimension,
35
// so we don't need to use __VARIANT__ to get extra coverage.
36
export const debugRenderPhaseSideEffectsForStrictMode = __DEV__;
packages/shared/forks/ReactFeatureFlags.www.js
+1
@@ -26,6 +26,7 @@ export const {
26
enableFilterEmptyStringAttributesDOM,
27
enableLegacyFBSupport,
28
enableDebugTracing,
29
+ deferRenderPhaseUpdateToNextBatch,
30
} = dynamicFeatureFlags;
31
32
// On WWW, __EXPERIMENTAL__ is used for a new modern build.