From daa0042517055cde264e0117f483d93c2e04142b Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 30 Mar 2023 14:10:19 -0400 Subject: [PATCH] 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. --- .../src/ReactFiberWorkLoop.js | 20 ++------- .../__tests__/ReactIncrementalUpdates-test.js | 44 +++++-------------- .../ReactCoffeeScriptClass-test.coffee | 4 +- .../react/src/__tests__/ReactES6Class-test.js | 4 +- .../__tests__/ReactTypeScriptClass-test.ts | 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.native.js | 1 - .../ReactFeatureFlags.test-renderer.www.js | 1 - .../forks/ReactFeatureFlags.www-dynamic.js | 10 ----- .../shared/forks/ReactFeatureFlags.www.js | 1 - 13 files changed, 16 insertions(+), 84 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 7db4c49cbf338..32ffdbfefdfa2 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -30,7 +30,6 @@ import { enableProfilerCommitHooks, enableProfilerNestedUpdatePhase, enableProfilerNestedUpdateScheduledHook, - deferRenderPhaseUpdateToNextBatch, enableDebugTracing, enableSchedulingProfiler, disableSchedulerTimeoutInWorkLoop, @@ -636,7 +635,6 @@ export function requestUpdateLane(fiber: Fiber): Lane { if ((mode & ConcurrentMode) === NoMode) { return (SyncLane: Lane); } else if ( - !deferRenderPhaseUpdateToNextBatch && (executionContext & RenderContext) !== NoContext && workInProgressRootRenderLanes !== NoLanes ) { @@ -804,14 +802,8 @@ export function scheduleUpdateOnFiber( if (root === workInProgressRoot) { // Received an update to a tree that's in the middle of rendering. Mark - // 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 - ) { + // that there was an interleaved update work on this root. + if ((executionContext & RenderContext) === NoContext) { workInProgressRootInterleavedUpdatedLanes = mergeLanes( workInProgressRootInterleavedUpdatedLanes, lane, @@ -870,13 +862,7 @@ export function scheduleInitialHydrationOnRoot( export function isUnsafeClassRenderPhaseUpdate(fiber: Fiber): boolean { // Check if this is a render phase update. Only called by class components, // which special (deprecated) behavior for UNSAFE_componentWillReceive props. - return ( - // TODO: Remove outdated deferRenderPhaseUpdateToNextBatch experiment. We - // decided not to enable it. - (!deferRenderPhaseUpdateToNextBatch || - (fiber.mode & ConcurrentMode) === NoMode) && - (executionContext & RenderContext) !== NoContext - ); + return (executionContext & RenderContext) !== NoContext; } // Use this function to schedule a task for a root. There's only one task per diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js index a85cf89c68ca4..7413e51a3ab14 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js @@ -387,11 +387,7 @@ describe('ReactIncrementalUpdates', () => { expect(instance.state).toEqual({a: 'a', b: 'b'}); - if (gate(flags => flags.deferRenderPhaseUpdateToNextBatch)) { - assertLog(['componentWillReceiveProps', 'render', 'render']); - } else { - assertLog(['componentWillReceiveProps', 'render']); - } + assertLog(['componentWillReceiveProps', 'render']); }); it('updates triggered from inside a class setState updater', async () => { @@ -419,26 +415,12 @@ describe('ReactIncrementalUpdates', () => { await expect( async () => - await waitForAll( - gate(flags => - flags.deferRenderPhaseUpdateToNextBatch - ? [ - 'setState updater', - // 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. - 'render', - 'render', - ] - : [ - 'setState updater', - // In the old reconciler, updates in the render phase receive - // the currently rendering expiration time, so the update - // flushes immediately in the same render. - 'render', - ], - ), - ), + await waitForAll([ + 'setState updater', + // Updates in the render phase receive the currently rendering + // lane, so the update flushes immediately in the same render. + 'render', + ]), ).toErrorDev( 'An update (setState, replaceState, or forceUpdate) was scheduled ' + 'from inside an update function. Update functions should be pure, ' + @@ -454,15 +436,9 @@ describe('ReactIncrementalUpdates', () => { }); await waitForAll( gate(flags => - 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. - ['render', 'render'] - : // In the old reconciler, updates in the render phase receive - // the currently rendering expiration time, so the update flushes - // immediately in the same render. - ['render'], + // Updates in the render phase receive the currently rendering + // lane, so the update flushes immediately in the same render. + ['render'], ), ); }); diff --git a/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee b/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee index 78a7ecafd9799..96bd3e0c24ca1 100644 --- a/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee +++ b/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee @@ -264,9 +264,7 @@ describe 'ReactCoffeeScriptClass', -> React.createElement('span', className: @state.bar) test React.createElement(Foo, initialValue: 'foo'), 'SPAN', 'bar' - # This is broken with deferRenderPhaseUpdateToNextBatch flag on. - # We can't use the gate feature here because this test is also in CoffeeScript and TypeScript. - expect(renderCount).toBe(if global.__WWW__ and !global.__VARIANT__ then 2 else 1) + expect(renderCount).toBe(1) it 'should warn with non-object in the initial state property', -> [['an array'], 'a string', 1234].forEach (state) -> diff --git a/packages/react/src/__tests__/ReactES6Class-test.js b/packages/react/src/__tests__/ReactES6Class-test.js index 72a632f4dd42a..1f2d899f88207 100644 --- a/packages/react/src/__tests__/ReactES6Class-test.js +++ b/packages/react/src/__tests__/ReactES6Class-test.js @@ -295,9 +295,7 @@ describe('ReactES6Class', () => { } } test(, 'SPAN', 'bar'); - // This is broken with deferRenderPhaseUpdateToNextBatch flag on. - // We can't use the gate feature here because this test is also in CoffeeScript and TypeScript. - expect(renderCount).toBe(global.__WWW__ && !global.__VARIANT__ ? 2 : 1); + expect(renderCount).toBe(1); }); it('should warn with non-object in the initial state property', () => { diff --git a/packages/react/src/__tests__/ReactTypeScriptClass-test.ts b/packages/react/src/__tests__/ReactTypeScriptClass-test.ts index f356fd08b609c..6f0dc668f5be7 100644 --- a/packages/react/src/__tests__/ReactTypeScriptClass-test.ts +++ b/packages/react/src/__tests__/ReactTypeScriptClass-test.ts @@ -521,9 +521,7 @@ describe('ReactTypeScriptClass', function() { it('renders only once when setting state in componentWillMount', function() { renderCount = 0; test(React.createElement(RenderOnce, {initialValue: 'foo'}), 'SPAN', 'bar'); - // This is broken with deferRenderPhaseUpdateToNextBatch flag on. - // We can't use the gate feature in TypeScript. - expect(renderCount).toBe(global.__WWW__ && !global.__VARIANT__ ? 2 : 1); + expect(renderCount).toBe(1); }); it('should warn with non-object in the initial state property', function() { diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 03a9aa32698c6..7a77a48f4eec3 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -152,14 +152,6 @@ export const enableUnifiedSyncLane = __EXPERIMENTAL__; // startTransition. Only relevant when enableSyncDefaultUpdates is disabled. export const allowConcurrentByDefault = false; -// Updates that occur in the render phase are not officially supported. But when -// they do occur, 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. Remove this flag once we have migrated to the -// new behavior. -// NOTE: Not sure if we'll end up doing this or not. -export const deferRenderPhaseUpdateToNextBatch = false; - // ----------------------------------------------------------------------------- // React DOM Chopping Block // diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 91a88e1432595..073f2f2041396 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -60,7 +60,6 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const skipUnmountedBoundaries = false; export const enableGetInspectorDataForInstanceInProduction = true; -export const deferRenderPhaseUpdateToNextBatch = false; export const createRootStrictEffectsByDefault = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index 106703103584d..e1cae707580f9 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -50,7 +50,6 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const skipUnmountedBoundaries = false; export const enableGetInspectorDataForInstanceInProduction = false; -export const deferRenderPhaseUpdateToNextBatch = false; export const createRootStrictEffectsByDefault = false; export const enableUseRefAccessWarning = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index ba3a6de6b6ba8..06dc5c495d68c 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -50,7 +50,6 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const skipUnmountedBoundaries = false; export const enableGetInspectorDataForInstanceInProduction = false; -export const deferRenderPhaseUpdateToNextBatch = false; export const createRootStrictEffectsByDefault = false; export const enableUseRefAccessWarning = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js index 72fbe79a09856..bc8fc2630a81f 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -41,7 +41,6 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const skipUnmountedBoundaries = false; export const enableGetInspectorDataForInstanceInProduction = false; -export const deferRenderPhaseUpdateToNextBatch = false; export const enableSuspenseAvoidThisFallback = false; export const enableSuspenseAvoidThisFallbackFizz = false; export const enableCPUSuspense = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index 16abbea0bc0f5..98ba1f4a1ed1e 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -50,7 +50,6 @@ export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = true; export const skipUnmountedBoundaries = false; export const enableGetInspectorDataForInstanceInProduction = false; -export const deferRenderPhaseUpdateToNextBatch = false; export const createRootStrictEffectsByDefault = false; export const enableUseRefAccessWarning = false; diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js index ef76f9129d1b0..d567eedb9755d 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -35,16 +35,6 @@ export const enableDebugTracing = __EXPERIMENTAL__; export const enableSchedulingProfiler = __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 replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__; diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index fc0b59ddfc65d..762b96c2cf08a 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -21,7 +21,6 @@ export const { revertRemovalOfSiblingPrerendering, replayFailedUnitOfWorkWithInvokeGuardedCallback, enableLegacyFBSupport, - deferRenderPhaseUpdateToNextBatch, enableDebugTracing, skipUnmountedBoundaries, enableUseRefAccessWarning,