@samitouri / QOS-React-2 / commits / 2d51251e60

Clean up deferRenderPhaseUpdateToNextBatch (#26511)

This is a change to some undefined behavior that we though we would do at one point but decided not to roll out. It's already disabled everywhere, so this just deletes the branch from the implementation and the tests.

Andrew Clark committed Mar 30, 2023 at 14:10 UTC 2d51251e608b7b1a8baf79ae6bdba81ed8e1939a
13 files changed +16 -84
packages/react-reconciler/src/ReactFiberWorkLoop.js
+3 -17
@@ -30,7 +30,6 @@ import {
30 enableProfilerCommitHooks,
31 enableProfilerNestedUpdatePhase,
32 enableProfilerNestedUpdateScheduledHook,
33 - deferRenderPhaseUpdateToNextBatch,
33 enableDebugTracing,
34 enableSchedulingProfiler,
35 disableSchedulerTimeoutInWorkLoop,
@@ -636,7 +635,6 @@ export function requestUpdateLane(fiber: Fiber): Lane {
635 if ((mode & ConcurrentMode) === NoMode) {
636 return (SyncLane: Lane);
637 } else if (
639 - !deferRenderPhaseUpdateToNextBatch &&
638 (executionContext & RenderContext) !== NoContext &&
639 workInProgressRootRenderLanes !== NoLanes
640 ) {
@@ -804,14 +802,8 @@ export function scheduleUpdateOnFiber(
802
803 if (root === workInProgressRoot) {
804 // Received an update to a tree that's in the middle of rendering. Mark
807 - // that there was an interleaved update work on this root. Unless the
808 - // `deferRenderPhaseUpdateToNextBatch` flag is off and this is a render
809 - // phase update. In that case, we don't treat render phase updates as if
810 - // they were interleaved, for backwards compat reasons.
811 - if (
812 - deferRenderPhaseUpdateToNextBatch ||
813 - (executionContext & RenderContext) === NoContext
814 - ) {
805 + // that there was an interleaved update work on this root.
806 + if ((executionContext & RenderContext) === NoContext) {
807 workInProgressRootInterleavedUpdatedLanes = mergeLanes(
808 workInProgressRootInterleavedUpdatedLanes,
809 lane,
@@ -870,13 +862,7 @@ export function scheduleInitialHydrationOnRoot(
862 export function isUnsafeClassRenderPhaseUpdate(fiber: Fiber): boolean {
863 // Check if this is a render phase update. Only called by class components,
864 // which special (deprecated) behavior for UNSAFE_componentWillReceive props.
873 - return (
874 - // TODO: Remove outdated deferRenderPhaseUpdateToNextBatch experiment. We
875 - // decided not to enable it.
876 - (!deferRenderPhaseUpdateToNextBatch ||
877 - (fiber.mode & ConcurrentMode) === NoMode) &&
878 - (executionContext & RenderContext) !== NoContext
879 - );
865 + return (executionContext & RenderContext) !== NoContext;
866 }
867
868 // Use this function to schedule a task for a root. There's only one task per
packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js
+10 -34
@@ -387,11 +387,7 @@ describe('ReactIncrementalUpdates', () => {
387
388 expect(instance.state).toEqual({a: 'a', b: 'b'});
389
390 - if (gate(flags => flags.deferRenderPhaseUpdateToNextBatch)) {
391 - assertLog(['componentWillReceiveProps', 'render', 'render']);
392 - } else {
393 - assertLog(['componentWillReceiveProps', 'render']);
394 - }
390 + assertLog(['componentWillReceiveProps', 'render']);
391 });
392
393 it('updates triggered from inside a class setState updater', async () => {
@@ -419,26 +415,12 @@ describe('ReactIncrementalUpdates', () => {
415
416 await expect(
417 async () =>
422 - await waitForAll(
423 - gate(flags =>
424 - flags.deferRenderPhaseUpdateToNextBatch
425 - ? [
426 - 'setState updater',
427 - // In the new reconciler, updates inside the render phase are
428 - // treated as if they came from an event, so the update gets
429 - // shifted to a subsequent render.
430 - 'render',
431 - 'render',
432 - ]
433 - : [
434 - 'setState updater',
435 - // In the old reconciler, updates in the render phase receive
436 - // the currently rendering expiration time, so the update
437 - // flushes immediately in the same render.
438 - 'render',
439 - ],
440 - ),
441 - ),
418 + await waitForAll([
419 + 'setState updater',
420 + // Updates in the render phase receive the currently rendering
421 + // lane, so the update flushes immediately in the same render.
422 + 'render',
423 + ]),
424 ).toErrorDev(
425 'An update (setState, replaceState, or forceUpdate) was scheduled ' +
426 'from inside an update function. Update functions should be pure, ' +
@@ -454,15 +436,9 @@ describe('ReactIncrementalUpdates', () => {
436 });
437 await waitForAll(
438 gate(flags =>
457 - flags.deferRenderPhaseUpdateToNextBatch
458 - ? // In the new reconciler, updates inside the render phase are
459 - // treated as if they came from an event, so the update gets shifted
460 - // to a subsequent render.
461 - ['render', 'render']
462 - : // In the old reconciler, updates in the render phase receive
463 - // the currently rendering expiration time, so the update flushes
464 - // immediately in the same render.
465 - ['render'],
439 + // Updates in the render phase receive the currently rendering
440 + // lane, so the update flushes immediately in the same render.
441 + ['render'],
442 ),
443 );
444 });
packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee
+1 -3
@@ -264,9 +264,7 @@ describe 'ReactCoffeeScriptClass', ->
264 React.createElement('span', className: @state.bar)
265
266 test React.createElement(Foo, initialValue: 'foo'), 'SPAN', 'bar'
267 - # This is broken with deferRenderPhaseUpdateToNextBatch flag on.
268 - # We can't use the gate feature here because this test is also in CoffeeScript and TypeScript.
269 - expect(renderCount).toBe(if global.__WWW__ and !global.__VARIANT__ then 2 else 1)
267 + expect(renderCount).toBe(1)
268
269 it 'should warn with non-object in the initial state property', ->
270 [['an array'], 'a string', 1234].forEach (state) ->
packages/react/src/__tests__/ReactES6Class-test.js
+1 -3
@@ -295,9 +295,7 @@ describe('ReactES6Class', () => {
295 }
296 }
297 test(<Foo initialValue="foo" />, 'SPAN', 'bar');
298 - // This is broken with deferRenderPhaseUpdateToNextBatch flag on.
299 - // We can't use the gate feature here because this test is also in CoffeeScript and TypeScript.
300 - expect(renderCount).toBe(global.__WWW__ && !global.__VARIANT__ ? 2 : 1);
298 + expect(renderCount).toBe(1);
299 });
300
301 it('should warn with non-object in the initial state property', () => {
packages/react/src/__tests__/ReactTypeScriptClass-test.ts
+1 -3
@@ -521,9 +521,7 @@ describe('ReactTypeScriptClass', function() {
521 it('renders only once when setting state in componentWillMount', function() {
522 renderCount = 0;
523 test(React.createElement(RenderOnce, {initialValue: 'foo'}), 'SPAN', 'bar');
524 - // This is broken with deferRenderPhaseUpdateToNextBatch flag on.
525 - // We can't use the gate feature in TypeScript.
526 - expect(renderCount).toBe(global.__WWW__ && !global.__VARIANT__ ? 2 : 1);
524 + expect(renderCount).toBe(1);
525 });
526
527 it('should warn with non-object in the initial state property', function() {
packages/shared/ReactFeatureFlags.js
-8
@@ -152,14 +152,6 @@ export const enableUnifiedSyncLane = __EXPERIMENTAL__;
152 // startTransition. Only relevant when enableSyncDefaultUpdates is disabled.
153 export const allowConcurrentByDefault = false;
154
155 -// Updates that occur in the render phase are not officially supported. But when
156 -// they do occur, we defer them to a subsequent render by picking a lane that's
157 -// not currently rendering. We treat them the same as if they came from an
158 -// interleaved event. Remove this flag once we have migrated to the
159 -// new behavior.
160 -// NOTE: Not sure if we'll end up doing this or not.
161 -export const deferRenderPhaseUpdateToNextBatch = false;
162 -
155 // -----------------------------------------------------------------------------
156 // React DOM Chopping Block
157 //
packages/shared/forks/ReactFeatureFlags.native-fb.js
-1
@@ -60,7 +60,6 @@ export const enableLegacyFBSupport = false;
60 export const enableFilterEmptyStringAttributesDOM = false;
61 export const skipUnmountedBoundaries = false;
62 export const enableGetInspectorDataForInstanceInProduction = true;
63 -export const deferRenderPhaseUpdateToNextBatch = false;
63
64 export const createRootStrictEffectsByDefault = false;
65
packages/shared/forks/ReactFeatureFlags.native-oss.js
-1
@@ -50,7 +50,6 @@ export const enableLegacyFBSupport = false;
50 export const enableFilterEmptyStringAttributesDOM = false;
51 export const skipUnmountedBoundaries = false;
52 export const enableGetInspectorDataForInstanceInProduction = false;
53 -export const deferRenderPhaseUpdateToNextBatch = false;
53
54 export const createRootStrictEffectsByDefault = false;
55 export const enableUseRefAccessWarning = false;
packages/shared/forks/ReactFeatureFlags.test-renderer.js
-1
@@ -50,7 +50,6 @@ export const enableLegacyFBSupport = false;
50 export const enableFilterEmptyStringAttributesDOM = false;
51 export const skipUnmountedBoundaries = false;
52 export const enableGetInspectorDataForInstanceInProduction = false;
53 -export const deferRenderPhaseUpdateToNextBatch = false;
53
54 export const createRootStrictEffectsByDefault = false;
55 export const enableUseRefAccessWarning = false;
packages/shared/forks/ReactFeatureFlags.test-renderer.native.js
-1
@@ -41,7 +41,6 @@ export const enableLegacyFBSupport = false;
41 export const enableFilterEmptyStringAttributesDOM = false;
42 export const skipUnmountedBoundaries = false;
43 export const enableGetInspectorDataForInstanceInProduction = false;
44 -export const deferRenderPhaseUpdateToNextBatch = false;
44 export const enableSuspenseAvoidThisFallback = false;
45 export const enableSuspenseAvoidThisFallbackFizz = false;
46 export const enableCPUSuspense = false;
packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
-1
@@ -50,7 +50,6 @@ export const enableLegacyFBSupport = false;
50 export const enableFilterEmptyStringAttributesDOM = true;
51 export const skipUnmountedBoundaries = false;
52 export const enableGetInspectorDataForInstanceInProduction = false;
53 -export const deferRenderPhaseUpdateToNextBatch = false;
53
54 export const createRootStrictEffectsByDefault = false;
55 export const enableUseRefAccessWarning = false;
packages/shared/forks/ReactFeatureFlags.www-dynamic.js
-10
@@ -35,16 +35,6 @@ export const enableDebugTracing = __EXPERIMENTAL__;
35
36 export const enableSchedulingProfiler = __VARIANT__;
37
38 -// This only has an effect in the new reconciler. But also, the new reconciler
39 -// is only enabled when __VARIANT__ is true. So this is set to the opposite of
40 -// __VARIANT__ so that it's `false` when running against the new reconciler.
41 -// Ideally we would test both against the new reconciler, but until then, we
42 -// should test the value that is used in www. Which is `false`.
43 -//
44 -// Once Lanes has landed in both reconciler forks, we'll get coverage of
45 -// both branches.
46 -export const deferRenderPhaseUpdateToNextBatch = !__VARIANT__;
47 -
38 // These are already tested in both modes using the build type dimension,
39 // so we don't need to use __VARIANT__ to get extra coverage.
40 export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__;
packages/shared/forks/ReactFeatureFlags.www.js
-1
@@ -21,7 +21,6 @@ export const {
21 revertRemovalOfSiblingPrerendering,
22 replayFailedUnitOfWorkWithInvokeGuardedCallback,
23 enableLegacyFBSupport,
24 - deferRenderPhaseUpdateToNextBatch,
24 enableDebugTracing,
25 skipUnmountedBoundaries,
26 enableUseRefAccessWarning,