Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clean up deferRenderPhaseUpdateToNextBatch #26511

Merged
merged 1 commit into from
Mar 30, 2023
Merged

Clean up deferRenderPhaseUpdateToNextBatch #26511

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 3 additions & 17 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
View file
Open in desktop
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) {
Copy link

@gabrielduete gabrielduete Mar 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(question): As much as comments were inserted in relation to this condition, what do you think about inserting a variable to store this condition? But with a nomenclature that fits the context.

Something like: conditionAutoExplanatory = (executionContext & RenderContext) === NoContext

More infos: juntossomosmais/frontend-guideline#63-descriptive-validations-if

PS: I'm new to open-source, so I don't know if I'm saying something wrong

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think that's a good practice sometimes. In this particular case, though, I think the comment would be necessary to provide additional context even if there were a descriptive variable name. So if the comment is going to be there regardless I'd rather keep the check inline.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it! Thanks for clarifying!

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
44 changes: 10 additions & 34 deletions packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js
View file
Open in desktop
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
4 changes: 1 addition & 3 deletions packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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.native.js
View file
Open in desktop
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
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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