Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,130 @@ describe('ReactFabric', () => {
expect(nativeFabricUIManager.completeRoot).toBeCalled();
});

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

const ComponentWithEffect = () => {
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();
});

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

const ComponentWithRef = () => {
React.useInsertionEffect(() => {});
return null;
};

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

await act(() =>
ReactFabric.render(
<View>
<ComponentWithRef />
</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();
});

// @gate enablePersistedModeClonedFlag
it('should not clone nodes when useImperativeHandle is used', async () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {foo: true},
uiViewClassName: 'RCTView',
}));

const ComponentWithImperativeHandle = props => {
React.useImperativeHandle(props.ref, () => ({greet: () => 'hello'}));
return null;
};

const ref = React.createRef();

await act(() =>
ReactFabric.render(
<View>
<ComponentWithImperativeHandle ref={ref} />
</View>,
11,
),
);
expect(nativeFabricUIManager.completeRoot).toBeCalled();
expect(ref.current.greet()).toBe('hello');
jest.clearAllMocks();

await act(() =>
ReactFabric.render(
<View>
<ComponentWithImperativeHandle ref={ref} />
</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();
expect(ref.current.greet()).toBe('hello');
});

it('should call dispatchCommand for native refs', async () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {foo: true},
Expand Down
7 changes: 6 additions & 1 deletion packages/react-reconciler/src/ReactFiberCommitWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import type {
import {
alwaysThrottleRetries,
enableCreateEventHandleAPI,
enablePersistedModeClonedFlag,
enableProfilerTimer,
enableProfilerCommitHooks,
enableProfilerNestedUpdatePhase,
Expand Down Expand Up @@ -98,6 +99,7 @@ import {
ShouldSuspendCommit,
MaySuspendCommit,
FormReset,
Cloned,
} from './ReactFiberFlags';
import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber';
import {runWithFiberInDEV} from './ReactCurrentFiber';
Expand Down Expand Up @@ -2554,7 +2556,10 @@ function recursivelyTraverseMutationEffects(
}
}

if (parentFiber.subtreeFlags & MutationMask) {
if (
parentFiber.subtreeFlags &
(enablePersistedModeClonedFlag ? MutationMask | Cloned : MutationMask)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should Cloned just be part of the MutationMask?

) {
let child = parentFiber.child;
while (child !== null) {
if (__DEV__) {
Expand Down
45 changes: 35 additions & 10 deletions packages/react-reconciler/src/ReactFiberCompleteWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
enableLegacyHidden,
enableSuspenseCallback,
enableScopeAPI,
enablePersistedModeClonedFlag,
enableProfilerTimer,
enableCache,
enableTransitionTracing,
Expand Down Expand Up @@ -90,6 +91,7 @@ import {
MaySuspendCommit,
ScheduleRetry,
ShouldSuspendCommit,
Cloned,
} from './ReactFiberFlags';

import {
Expand Down Expand Up @@ -182,6 +184,16 @@ function markUpdate(workInProgress: Fiber) {
workInProgress.flags |= Update;
}

/**
* Tag the fiber with Cloned in persistent mode to signal that
* it received an update that requires a clone of the tree above.
*/
function markCloned(workInProgress: Fiber) {
if (supportsPersistence && enablePersistedModeClonedFlag) {
workInProgress.flags |= Cloned;
}
}

/**
* In persistent mode, return whether this update needs to clone the subtree.
*/
Expand All @@ -199,9 +211,12 @@ function doesRequireClone(current: null | Fiber, completedWork: Fiber) {
// then we only have to check the `completedWork.subtreeFlags`.
let child = completedWork.child;
while (child !== null) {
const checkedFlags = enablePersistedModeClonedFlag
? Cloned | Visibility | Placement | ChildDeletion
: MutationMask;
if (
(child.flags & MutationMask) !== NoFlags ||
(child.subtreeFlags & MutationMask) !== NoFlags
(child.flags & checkedFlags) !== NoFlags ||
(child.subtreeFlags & checkedFlags) !== NoFlags
) {
return true;
}
Expand Down Expand Up @@ -450,6 +465,7 @@ function updateHostComponent(

let newChildSet = null;
if (requiresClone && passChildrenWhenCloningPersistedNodes) {
markCloned(workInProgress);
newChildSet = createContainerChildSet();
// If children might have changed, we have to add them all to the set.
appendAllChildrenToContainer(
Expand All @@ -473,6 +489,8 @@ function updateHostComponent(
// Note that this might release a previous clone.
workInProgress.stateNode = currentInstance;
return;
} else {
markCloned(workInProgress);
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this right? seems like we should only mark clone if requiresClone is true

Copy link
Member Author

Choose a reason for hiding this comment

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

cloneInstance above returned a new instance since newInstance !== currentInstance in this branch, so the cloned tree needs to bubble up (I think)

}

// Certain renderers require commit-time effects for initial mount.
Expand All @@ -485,12 +503,14 @@ function updateHostComponent(
}
workInProgress.stateNode = newInstance;
if (!requiresClone) {
// If there are no other effects in this tree, we need to flag this node as having one.
// Even though we're not going to use it for anything.
// Otherwise parents won't know that there are new children to propagate upwards.
markUpdate(workInProgress);
if (!enablePersistedModeClonedFlag) {
// If there are no other effects in this tree, we need to flag this node as having one.
// Even though we're not going to use it for anything.
// Otherwise parents won't know that there are new children to propagate upwards.
markUpdate(workInProgress);
}
} else if (!passChildrenWhenCloningPersistedNodes) {
// If children might have changed, we have to add them all to the set.
// If children have changed, we have to add them all to the set.
appendAllChildren(
newInstance,
workInProgress,
Expand Down Expand Up @@ -618,15 +638,18 @@ function updateHostText(
// If the text content differs, we'll create a new text instance for it.
const rootContainerInstance = getRootHostContainer();
const currentHostContext = getHostContext();
markCloned(workInProgress);
workInProgress.stateNode = createTextInstance(
newText,
rootContainerInstance,
currentHostContext,
workInProgress,
);
// We'll have to mark it as having an effect, even though we won't use the effect for anything.
// This lets the parents know that at least one of their children has changed.
markUpdate(workInProgress);
if (!enablePersistedModeClonedFlag) {
// We'll have to mark it as having an effect, even though we won't use the effect for anything.
// This lets the parents know that at least one of their children has changed.
markUpdate(workInProgress);
}
Comment on lines +648 to +652
Copy link
Collaborator

Choose a reason for hiding this comment

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

This may be right but it's a little confusing that markCloned and markUpdate are not apparently mutually exclusive (when they actually are given the implementation of markCloned)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually the core of the PR. We can have an Update for example due to effect dependencies changing, but do not want to trigger a clone up the tree indicated by the Cloned flag.

Previously they were both grouped together which means we'd do a full subtree clone if there was an effect dependency change in the subtree.

Even with the feature flag an effect change will still set the Update flag, but Update (behind flag) no longer causes a clone.

} else {
workInProgress.stateNode = current.stateNode;
}
Expand Down Expand Up @@ -1229,6 +1252,7 @@ function completeWork(
);
// TODO: For persistent renderers, we should pass children as part
// of the initial instance creation
markCloned(workInProgress);
appendAllChildren(instance, workInProgress, false, false);
workInProgress.stateNode = instance;

Expand Down Expand Up @@ -1284,6 +1308,7 @@ function completeWork(
if (wasHydrated) {
prepareToHydrateHostTextInstance(workInProgress);
} else {
markCloned(workInProgress);
workInProgress.stateNode = createTextInstance(
newText,
rootContainerInstance,
Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiberFlags.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export const Hydrating = /* */ 0b0000000000000001000000000000

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

export const ChildDeletion = /* */ 0b0000000000000000000000010000;
export const ContentReset = /* */ 0b0000000000000000000000100000;
Expand Down
26 changes: 24 additions & 2 deletions packages/react-reconciler/src/__tests__/ReactPersistent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@

let React;
let ReactNoopPersistent;

let act;
let waitForAll;

describe('ReactPersistent', () => {
Expand All @@ -20,8 +22,7 @@ describe('ReactPersistent', () => {

React = require('react');
ReactNoopPersistent = require('react-noop-renderer/persistent');
const InternalTestUtils = require('internal-test-utils');
waitForAll = InternalTestUtils.waitForAll;
({act, waitForAll} = require('internal-test-utils'));
});

// Inlined from shared folder so we can run this test on a bundle.
Expand Down Expand Up @@ -213,4 +214,25 @@ describe('ReactPersistent', () => {
// The original is unchanged.
expect(newPortalChildren).toEqual([div(span(), 'Hello ', 'World')]);
});

it('remove children', async () => {
function Wrapper({children}) {
return children;
}

const root = ReactNoopPersistent.createRoot();
await act(() => {
root.render(
<Wrapper>
<inner />
</Wrapper>,
);
});
expect(root.getChildrenAsJSX()).toEqual(<inner />);

await act(() => {
root.render(<Wrapper />);
});
expect(root.getChildrenAsJSX()).toEqual(null);
});
});
6 changes: 6 additions & 0 deletions packages/shared/ReactFeatureFlags.js
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,12 @@ export const passChildrenWhenCloningPersistedNodes = false;

export const enableServerComponentLogs = __EXPERIMENTAL__;

/**
* Enables a new Fiber flag used in persisted mode to reduce the number
* of cloned host components.
*/
export const enablePersistedModeClonedFlag = false;

export const enableAddPropertiesFastPath = false;

export const enableOwnerStacks = __EXPERIMENTAL__;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
export const alwaysThrottleRetries = __VARIANT__;
export const enableAddPropertiesFastPath = __VARIANT__;
export const enableObjectFiber = __VARIANT__;
export const enablePersistedModeClonedFlag = __VARIANT__;
export const enableShallowPropDiffing = __VARIANT__;
export const passChildrenWhenCloningPersistedNodes = __VARIANT__;
export const enableFabricCompleteRootInCommitPhase = __VARIANT__;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-fb.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export const {
enableAddPropertiesFastPath,
enableFabricCompleteRootInCommitPhase,
enableObjectFiber,
enablePersistedModeClonedFlag,
enableShallowPropDiffing,
passChildrenWhenCloningPersistedNodes,
enableLazyContextPropagation,
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-oss.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ export const enableLegacyHidden = false;
export const enableNoCloningMemoCache = false;
export const enableObjectFiber = false;
export const enableOwnerStacks = false;
export const enablePersistedModeClonedFlag = false;
export const enablePostpone = false;
export const enableReactTestRendererWarning = false;
export const enableRefAsProp = true;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.test-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ export const enableAsyncActions = true;
export const alwaysThrottleRetries = true;

export const passChildrenWhenCloningPersistedNodes = false;
export const enablePersistedModeClonedFlag = false;
export const enableUseDeferredValueInitialArg = __EXPERIMENTAL__;
export const disableClientCache = true;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ export const enableLegacyHidden = false;
export const enableNoCloningMemoCache = false;
export const enableObjectFiber = false;
export const enableOwnerStacks = false;
export const enablePersistedModeClonedFlag = false;
export const enablePostpone = false;
export const enableProfilerCommitHooks = __PROFILE__;
export const enableProfilerNestedUpdatePhase = __PROFILE__;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ export const enableAsyncActions = true;
export const alwaysThrottleRetries = true;

export const passChildrenWhenCloningPersistedNodes = false;
export const enablePersistedModeClonedFlag = false;
export const enableUseDeferredValueInitialArg = true;
export const disableClientCache = true;

Expand Down
Loading