From fc3ac3af4c375a9cd3393acdf85a9af2eb2922be Mon Sep 17 00:00:00 2001 From: Jan Kassens Date: Mon, 16 Oct 2023 16:45:39 -0400 Subject: [PATCH] Split Update flag into a mutation and effect flag The `Update` flag was overloaded to mean that there's "work to do during the commit phase". On the one side that was updates to host components. On the other side that meant running effects. The flag was also used in persistent mode to trigger cloning the immutable view hierarchy to perform the updates. When the flag was only used to indicate calling effects, this meant needlessly cloning the subtree. Here we *start* splitting up the flag into 2. In the transition phase we set both flags in some places, but most likey every location should only set or check one of the flags. --- .../__tests__/ReactFabric-test.internal.js | 40 +++++++++++++++++++ .../src/ReactFiberCommitWork.js | 5 ++- .../react-reconciler/src/ReactFiberFlags.js | 24 ++++++++++- .../react-reconciler/src/ReactFiberHooks.js | 20 +++++----- .../src/ReactFiberWorkLoop.js | 13 +++++- 5 files changed, 86 insertions(+), 16 deletions(-) diff --git a/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js b/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js index 644d7297403..9fbec9f5b04 100644 --- a/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js +++ b/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js @@ -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` + React.useLayoutEffect(() => {}); + return null; + }; + + await act(() => + ReactFabric.render( + + + , + 11, + ), + ); + expect(nativeFabricUIManager.completeRoot).toBeCalled(); + jest.clearAllMocks(); + + await act(() => + ReactFabric.render( + + + , + 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}, diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 73889d8b818..c383f4acaab 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -97,6 +97,7 @@ import { Visibility, ShouldSuspendCommit, MaySuspendCommit, + Effect, } from './ReactFiberFlags'; import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber'; import { @@ -1062,7 +1063,7 @@ function commitLayoutEffectOnFiber( finishedWork, committedLanes, ); - if (flags & Update) { + if (flags & Effect) { commitHookLayoutEffects(finishedWork, HookLayout | HookHasEffect); } break; @@ -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); diff --git a/packages/react-reconciler/src/ReactFiberFlags.js b/packages/react-reconciler/src/ReactFiberFlags.js index 5febfa3d528..92f40d0ffd4 100644 --- a/packages/react-reconciler/src/ReactFiberFlags.js +++ b/packages/react-reconciler/src/ReactFiberFlags.js @@ -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; @@ -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; @@ -89,7 +109,7 @@ export const BeforeMutationMask: number = export const MutationMask = Placement | - Update | + HostComponentUpdate | ChildDeletion | ContentReset | Ref | diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index d2c456a5f9c..48a3a376f45 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -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, @@ -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); } @@ -2398,7 +2398,7 @@ function updateEffect( function useEffectEventImpl) => Return>( payload: EventFunctionPayload, ) { - currentlyRenderingFiber.flags |= UpdateEffect; + currentlyRenderingFiber.flags |= EffectEffect; let componentUpdateQueue: null | FunctionComponentUpdateQueue = (currentlyRenderingFiber.updateQueue: any); if (componentUpdateQueue === null) { @@ -2453,21 +2453,21 @@ function mountInsertionEffect( create: () => (() => void) | void, deps: Array | void | null, ): void { - mountEffectImpl(UpdateEffect, HookInsertion, create, deps); + mountEffectImpl(EffectEffect, HookInsertion, create, deps); } function updateInsertionEffect( create: () => (() => void) | void, deps: Array | void | null, ): void { - return updateEffectImpl(UpdateEffect, HookInsertion, create, deps); + return updateEffectImpl(EffectEffect, HookInsertion, create, deps); } function mountLayoutEffect( create: () => (() => void) | void, deps: Array | void | null, ): void { - let fiberFlags: Flags = UpdateEffect | LayoutStaticEffect; + let fiberFlags: Flags = EffectEffect | LayoutStaticEffect; if ( __DEV__ && (currentlyRenderingFiber.mode & StrictEffectsMode) !== NoMode @@ -2481,7 +2481,7 @@ function updateLayoutEffect( create: () => (() => void) | void, deps: Array | void | null, ): void { - return updateEffectImpl(UpdateEffect, HookLayout, create, deps); + return updateEffectImpl(EffectEffect, HookLayout, create, deps); } function imperativeHandleEffect( @@ -2533,7 +2533,7 @@ function mountImperativeHandle( 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 @@ -2568,7 +2568,7 @@ function updateImperativeHandle( deps !== null && deps !== undefined ? deps.concat([ref]) : null; updateEffectImpl( - UpdateEffect, + EffectEffect, HookLayout, imperativeHandleEffect.bind(null, create, ref), effectDeps, diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index cb217d04eba..57b5dc1a350 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -124,6 +124,7 @@ import { Visibility, MountPassiveDev, MountLayoutDev, + Effect, } from './ReactFiberFlags'; import { NoLanes, @@ -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) {