Skip to content

Split Update flag into a mutation and effect flag #27533

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

Closed
wants to merge 1 commit into from
Closed

Split Update flag into a mutation and effect flag #27533

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
40 changes: 40 additions & 0 deletions packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,46 @@ describe('ReactFabric', () => {
expect(nativeFabricUIManager.completeRoot).toBeCalled();
});

it('should not clone nodes when layout effects are used', async () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {foo: true},
uiViewClassName: 'RCTView',
}));

const ComponentWithEffect = () => {
// Same thing happens with `ref` and `useImperativeHandle`
Copy link
Member

Choose a reason for hiding this comment

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

Worth testing that ref / useImperativeHandle now pass this test too?

React.useLayoutEffect(() => {});
return null;
};

await act(() =>
ReactFabric.render(
<View>
<ComponentWithEffect />
</View>,
11,
),
);
expect(nativeFabricUIManager.completeRoot).toBeCalled();
jest.clearAllMocks();

await act(() =>
ReactFabric.render(
<View>
<ComponentWithEffect />
</View>,
11,
),
);
expect(nativeFabricUIManager.cloneNode).not.toBeCalled();
expect(nativeFabricUIManager.cloneNodeWithNewChildren).not.toBeCalled();
expect(nativeFabricUIManager.cloneNodeWithNewProps).not.toBeCalled();
expect(
nativeFabricUIManager.cloneNodeWithNewChildrenAndProps,
).not.toBeCalled();
expect(nativeFabricUIManager.completeRoot).not.toBeCalled();
});

it('should call dispatchCommand for native refs', async () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {foo: true},
Expand Down
5 changes: 3 additions & 2 deletions packages/react-reconciler/src/ReactFiberCommitWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ import {
Visibility,
ShouldSuspendCommit,
MaySuspendCommit,
Effect,
} from './ReactFiberFlags';
import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber';
import {
Expand Down Expand Up @@ -1062,7 +1063,7 @@ function commitLayoutEffectOnFiber(
finishedWork,
committedLanes,
);
if (flags & Update) {
if (flags & Effect) {
commitHookLayoutEffects(finishedWork, HookLayout | HookHasEffect);
}
break;
Expand Down Expand Up @@ -2525,7 +2526,7 @@ function recursivelyTraverseMutationEffects(
}

const prevDebugFiber = getCurrentDebugFiberInDEV();
if (parentFiber.subtreeFlags & MutationMask) {
if (parentFiber.subtreeFlags & (MutationMask | Effect)) {
let child = parentFiber.child;
while (child !== null) {
setCurrentDebugFiberInDEV(child);
Expand Down
24 changes: 22 additions & 2 deletions packages/react-reconciler/src/ReactFiberFlags.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,24 @@ export const DidCapture = /* */ 0b0000000000000000000010000000
export const Hydrating = /* */ 0b0000000000000001000000000000;

// You can change the rest (and add more).
export const Update = /* */ 0b0000000000000000000000000100;

/**
* Work needs to be performed on host components, such as updating props.
*/
export const HostComponentUpdate = /* */ 0b0000000000000000000000000100;

/**
* Indicates presence of effects (e.g. InsertionEffect, LayoutEffect)
*/
export const Effect = /* */ 0b10000000000000000000000000000;

/**
* One of the two flags.
*
* @deprecated Should use the more specific flag to allow us to bail out in more cases.
*/
export const Update = HostComponentUpdate | Effect;

/* Skipped value: 0b0000000000000000000000001000; */

export const ChildDeletion = /* */ 0b0000000000000000000000010000;
Expand All @@ -33,6 +50,9 @@ export const Snapshot = /* */ 0b0000000000000000010000000000
export const Passive = /* */ 0b0000000000000000100000000000;
/* Used by Hydrating: 0b0000000000000001000000000000; */

/**
* Set when the visibility state of Activity/LegacyHidden changed.
*/
export const Visibility = /* */ 0b0000000000000010000000000000;
export const StoreConsistency = /* */ 0b0000000000000100000000000000;

Expand Down Expand Up @@ -89,7 +109,7 @@ export const BeforeMutationMask: number =

export const MutationMask =
Placement |
Update |
HostComponentUpdate |
ChildDeletion |
ContentReset |
Ref |
Expand Down
20 changes: 10 additions & 10 deletions packages/react-reconciler/src/ReactFiberHooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,10 @@ import {
Passive as PassiveEffect,
PassiveStatic as PassiveStaticEffect,
StaticMask as StaticMaskEffect,
Update as UpdateEffect,
StoreConsistency,
MountLayoutDev as MountLayoutDevEffect,
MountPassiveDev as MountPassiveDevEffect,
Effect as EffectEffect,
} from './ReactFiberFlags';
import {
HasEffect as HookHasEffect,
Expand Down Expand Up @@ -883,10 +883,10 @@ export function bailoutHooks(
MountPassiveDevEffect |
MountLayoutDevEffect |
PassiveEffect |
UpdateEffect
EffectEffect
);
} else {
workInProgress.flags &= ~(PassiveEffect | UpdateEffect);
workInProgress.flags &= ~(PassiveEffect | EffectEffect);
}
current.lanes = removeLanes(current.lanes, lanes);
}
Expand Down Expand Up @@ -2398,7 +2398,7 @@ function updateEffect(
function useEffectEventImpl<Args, Return, F: (...Array<Args>) => Return>(
payload: EventFunctionPayload<Args, Return, F>,
) {
currentlyRenderingFiber.flags |= UpdateEffect;
currentlyRenderingFiber.flags |= EffectEffect;
let componentUpdateQueue: null | FunctionComponentUpdateQueue =
(currentlyRenderingFiber.updateQueue: any);
if (componentUpdateQueue === null) {
Expand Down Expand Up @@ -2453,21 +2453,21 @@ function mountInsertionEffect(
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
mountEffectImpl(UpdateEffect, HookInsertion, create, deps);
mountEffectImpl(EffectEffect, HookInsertion, create, deps);
}

function updateInsertionEffect(
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
return updateEffectImpl(UpdateEffect, HookInsertion, create, deps);
return updateEffectImpl(EffectEffect, HookInsertion, create, deps);
}

function mountLayoutEffect(
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
let fiberFlags: Flags = UpdateEffect | LayoutStaticEffect;
let fiberFlags: Flags = EffectEffect | LayoutStaticEffect;
if (
__DEV__ &&
(currentlyRenderingFiber.mode & StrictEffectsMode) !== NoMode
Expand All @@ -2481,7 +2481,7 @@ function updateLayoutEffect(
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
return updateEffectImpl(UpdateEffect, HookLayout, create, deps);
return updateEffectImpl(EffectEffect, HookLayout, create, deps);
}

function imperativeHandleEffect<T>(
Expand Down Expand Up @@ -2533,7 +2533,7 @@ function mountImperativeHandle<T>(
const effectDeps =
deps !== null && deps !== undefined ? deps.concat([ref]) : null;

let fiberFlags: Flags = UpdateEffect | LayoutStaticEffect;
let fiberFlags: Flags = EffectEffect | LayoutStaticEffect;
if (
__DEV__ &&
(currentlyRenderingFiber.mode & StrictEffectsMode) !== NoMode
Expand Down Expand Up @@ -2568,7 +2568,7 @@ function updateImperativeHandle<T>(
deps !== null && deps !== undefined ? deps.concat([ref]) : null;

updateEffectImpl(
UpdateEffect,
EffectEffect,
HookLayout,
imperativeHandleEffect.bind(null, create, ref),
effectDeps,
Expand Down
13 changes: 11 additions & 2 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ import {
Visibility,
MountPassiveDev,
MountLayoutDev,
Effect,
} from './ReactFiberFlags';
import {
NoLanes,
Expand Down Expand Up @@ -2745,11 +2746,19 @@ function commitRootImpl(
// Reconsider whether this is necessary.
const subtreeHasEffects =
(finishedWork.subtreeFlags &
(BeforeMutationMask | MutationMask | LayoutMask | PassiveMask)) !==
(BeforeMutationMask |
MutationMask |
Effect |
LayoutMask |
PassiveMask)) !==
NoFlags;
const rootHasEffect =
(finishedWork.flags &
(BeforeMutationMask | MutationMask | LayoutMask | PassiveMask)) !==
(BeforeMutationMask |
MutationMask |
Effect |
LayoutMask |
PassiveMask)) !==
NoFlags;

if (subtreeHasEffects || rootHasEffect) {
Expand Down