Skip to content

Commit

Permalink
Clean up deferRenderPhaseUpdateToNextBatch (facebook#26511)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
acdlite authored and AndyPengc12 committed Apr 15, 2024
1 parent 8bf8ffa commit daa0042
Show file tree
Hide file tree
Showing 13 changed files with 16 additions and 84 deletions.
20 changes: 3 additions & 17 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import {
enableProfilerCommitHooks,
enableProfilerNestedUpdatePhase,
enableProfilerNestedUpdateScheduledHook,
deferRenderPhaseUpdateToNextBatch,
enableDebugTracing,
enableSchedulingProfiler,
disableSchedulerTimeoutInWorkLoop,
Expand Down Expand Up @@ -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
) {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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, ' +
Expand All @@ -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'],
),
);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) ->
Expand Down
4 changes: 1 addition & 3 deletions packages/react/src/__tests__/ReactES6Class-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -295,9 +295,7 @@ describe('ReactES6Class', () => {
}
}
test(<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(global.__WWW__ && !global.__VARIANT__ ? 2 : 1);
expect(renderCount).toBe(1);
});

it('should warn with non-object in the initial state property', () => {
Expand Down
4 changes: 1 addition & 3 deletions packages/react/src/__tests__/ReactTypeScriptClass-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
8 changes: 0 additions & 8 deletions packages/shared/ReactFeatureFlags.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
//
Expand Down
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.native-fb.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.native-oss.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.test-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
10 changes: 0 additions & 10 deletions packages/shared/forks/ReactFeatureFlags.www-dynamic.js
Original file line number Diff line number Diff line change
Expand Up @@ -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__;
Expand Down
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.www.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ export const {
revertRemovalOfSiblingPrerendering,
replayFailedUnitOfWorkWithInvokeGuardedCallback,
enableLegacyFBSupport,
deferRenderPhaseUpdateToNextBatch,
enableDebugTracing,
skipUnmountedBoundaries,
enableUseRefAccessWarning,
Expand Down

0 comments on commit daa0042

Please sign in to comment.