Skip to content

Commit

Permalink
[Fabric] Pass children when cloning (#27458)
Browse files Browse the repository at this point in the history
## Summary

Currently when cloning nodes in Fabric, we reset a node's children on
each clone, and then repeatedly call appendChild to restore the previous
list of children (even if it was quasi-identical to before). This causes
unnecessary invalidation of the layout state in Fabric's ShadowNode data
(which in turn may require additional yoga clones) and extra JSI calls.

This PR adds a feature flag to pass in the children as part of the clone
call, so Fabric always has a complete view of the node that's being
mutated.

This feature flag requires matching changes in the react-native repo:
facebook/react-native#39817

## How did you test this change?

Unit test added demonstrates the new behaviour 

```
yarn test -r www-modern ReactFabric-test
yarn test ReactFabric-test.internal
```

Tested a manual sync into React Native and verified core surfaces render
correctly.
  • Loading branch information
javache committed Oct 10, 2023
1 parent dddfe68 commit 151e75a
Show file tree
Hide file tree
Showing 16 changed files with 178 additions and 55 deletions.
50 changes: 37 additions & 13 deletions packages/react-native-renderer/src/ReactFiberConfigFabric.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,10 @@ const {
unstable_getCurrentEventPriority: fabricGetCurrentEventPriority,
} = nativeFabricUIManager;

import {useMicrotasksForSchedulingInFabric} from 'shared/ReactFeatureFlags';
import {
useMicrotasksForSchedulingInFabric,
passChildrenWhenCloningPersistedNodes,
} from 'shared/ReactFeatureFlags';

const {get: getViewConfigForType} = ReactNativeViewConfigRegistry;

Expand Down Expand Up @@ -87,7 +90,7 @@ export type TextInstance = {
export type HydratableInstance = Instance | TextInstance;
export type PublicInstance = ReactNativePublicInstance;
export type Container = number;
export type ChildSet = Object;
export type ChildSet = Object | Array<Node>;
export type HostContext = $ReadOnly<{
isInAParentText: boolean,
}>;
Expand Down Expand Up @@ -346,9 +349,8 @@ export function cloneInstance(
type: string,
oldProps: Props,
newProps: Props,
internalInstanceHandle: InternalInstanceHandle,
keepChildren: boolean,
recyclableInstance: null | Instance,
newChildSet: ?ChildSet,
): Instance {
const viewConfig = instance.canonical.viewConfig;
const updatePayload = diff(oldProps, newProps, viewConfig.validAttributes);
Expand All @@ -367,12 +369,26 @@ export function cloneInstance(
return instance;
}
} else {
if (updatePayload !== null) {
clone = cloneNodeWithNewChildrenAndProps(node, updatePayload);
// If passChildrenWhenCloningPersistedNodes is enabled, children will be non-null
if (newChildSet != null) {
if (updatePayload !== null) {
clone = cloneNodeWithNewChildrenAndProps(
node,
newChildSet,
updatePayload,
);
} else {
clone = cloneNodeWithNewChildren(node, newChildSet);
}
} else {
clone = cloneNodeWithNewChildren(node);
if (updatePayload !== null) {
clone = cloneNodeWithNewChildrenAndProps(node, updatePayload);
} else {
clone = cloneNodeWithNewChildren(node);
}
}
}

return {
node: clone,
canonical: instance.canonical,
Expand All @@ -383,7 +399,6 @@ export function cloneHiddenInstance(
instance: Instance,
type: string,
props: Props,
internalInstanceHandle: InternalInstanceHandle,
): Instance {
const viewConfig = instance.canonical.viewConfig;
const node = instance.node;
Expand All @@ -400,20 +415,27 @@ export function cloneHiddenInstance(
export function cloneHiddenTextInstance(
instance: Instance,
text: string,
internalInstanceHandle: InternalInstanceHandle,
): TextInstance {
throw new Error('Not yet implemented.');
}

export function createContainerChildSet(container: Container): ChildSet {
return createChildNodeSet(container);
export function createContainerChildSet(): ChildSet {
if (passChildrenWhenCloningPersistedNodes) {
return [];
} else {
return createChildNodeSet();
}
}

export function appendChildToContainerChildSet(
childSet: ChildSet,
child: Instance | TextInstance,
): void {
appendChildNodeToSet(childSet, child.node);
if (passChildrenWhenCloningPersistedNodes) {
childSet.push(child.node);
} else {
appendChildNodeToSet(childSet, child.node);
}
}

export function finalizeContainerChildren(
Expand All @@ -426,7 +448,9 @@ export function finalizeContainerChildren(
export function replaceContainerChildren(
container: Container,
newChildren: ChildSet,
): void {}
): void {
// Noop - children will be replaced in finalizeContainerChildren
}

export function getInstanceFromNode(node: any): empty {
throw new Error('Not yet implemented.');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,15 @@ const RCTFabricUIManager = {
children: node.children,
};
}),
cloneNodeWithNewChildren: jest.fn(function cloneNodeWithNewChildren(node) {
cloneNodeWithNewChildren: jest.fn(function cloneNodeWithNewChildren(
node,
children,
) {
return {
reactTag: node.reactTag,
viewName: node.viewName,
props: node.props,
children: [],
children: children ?? [],
};
}),
cloneNodeWithNewProps: jest.fn(function cloneNodeWithNewProps(
Expand All @@ -91,11 +94,17 @@ const RCTFabricUIManager = {
}),
cloneNodeWithNewChildrenAndProps: jest.fn(
function cloneNodeWithNewChildrenAndProps(node, newPropsDiff) {
let children = [];
if (arguments.length === 3) {
children = newPropsDiff;
newPropsDiff = arguments[2];
}

return {
reactTag: node.reactTag,
viewName: node.viewName,
props: {...node.props, ...newPropsDiff},
children: [],
children,
};
},
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -210,8 +210,13 @@ describe('ReactFabric', () => {
11,
);
});
const argIndex = gate(flags => flags.passChildrenWhenCloningPersistedNodes)
? 2
: 1;
expect(
nativeFabricUIManager.cloneNodeWithNewChildrenAndProps.mock.calls[0][1],
nativeFabricUIManager.cloneNodeWithNewChildrenAndProps.mock.calls[0][
argIndex
],
).toEqual({
foo: 'b',
});
Expand All @@ -220,6 +225,54 @@ describe('ReactFabric', () => {
).toMatchSnapshot();
});

it('should not clone nodes without children when updating props', async () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {foo: true},
uiViewClassName: 'RCTView',
}));

const Component = ({foo}) => (
<View>
<View foo={foo} />
</View>
);

await act(() => ReactFabric.render(<Component foo={true} />, 11));
expect(nativeFabricUIManager.completeRoot).toBeCalled();
jest.clearAllMocks();

await act(() => ReactFabric.render(<Component foo={false} />, 11));
expect(nativeFabricUIManager.cloneNode).not.toBeCalled();
expect(nativeFabricUIManager.cloneNodeWithNewProps).toHaveBeenCalledTimes(
1,
);
expect(nativeFabricUIManager.cloneNodeWithNewProps).toHaveBeenCalledWith(
expect.anything(),
{foo: false},
);

expect(
nativeFabricUIManager.cloneNodeWithNewChildren,
).toHaveBeenCalledTimes(1);
if (gate(flags => flags.passChildrenWhenCloningPersistedNodes)) {
expect(
nativeFabricUIManager.cloneNodeWithNewChildren,
).toHaveBeenCalledWith(expect.anything(), [
expect.objectContaining({props: {foo: false}}),
]);
expect(nativeFabricUIManager.appendChild).not.toBeCalled();
} else {
expect(
nativeFabricUIManager.cloneNodeWithNewChildren,
).toHaveBeenCalledWith(expect.anything());
expect(nativeFabricUIManager.appendChild).toHaveBeenCalledTimes(1);
}
expect(
nativeFabricUIManager.cloneNodeWithNewChildrenAndProps,
).not.toBeCalled();
expect(nativeFabricUIManager.completeRoot).toBeCalled();
});

it('should call dispatchCommand for native refs', async () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {foo: true},
Expand Down
21 changes: 4 additions & 17 deletions packages/react-noop-renderer/src/createReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -223,9 +223,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
type: string,
oldProps: Props,
newProps: Props,
internalInstanceHandle: Object,
keepChildren: boolean,
recyclableInstance: null | Instance,
children: ?$ReadOnlyArray<Instance>,
): Instance {
if (__DEV__) {
checkPropStringCoercion(newProps.children, 'children');
Expand All @@ -234,7 +233,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
id: instance.id,
type: type,
parent: instance.parent,
children: keepChildren ? instance.children : [],
children: keepChildren ? instance.children : children ?? [],
text: shouldSetTextContent(type, newProps)
? computeText((newProps.children: any) + '', instance.context)
: null,
Expand Down Expand Up @@ -704,9 +703,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
cloneInstance,
clearContainer,

createContainerChildSet(
container: Container,
): Array<Instance | TextInstance> {
createContainerChildSet(): Array<Instance | TextInstance> {
return [];
},

Expand Down Expand Up @@ -742,25 +739,15 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
instance: Instance,
type: string,
props: Props,
internalInstanceHandle: Object,
): Instance {
const clone = cloneInstance(
instance,
type,
props,
props,
internalInstanceHandle,
true,
null,
);
const clone = cloneInstance(instance, type, props, props, true, null);
clone.hidden = true;
return clone;
},

cloneHiddenTextInstance(
instance: TextInstance,
text: string,
internalInstanceHandle: Object,
): TextInstance {
const clone = {
text: instance.text,
Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiberCommitWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -1724,7 +1724,7 @@ function emptyPortalContainer(current: Fiber) {
...
} = current.stateNode;
const {containerInfo} = portal;
const emptyChildSet = createContainerChildSet(containerInfo);
const emptyChildSet = createContainerChildSet();
replaceContainerChildren(containerInfo, emptyChildSet);
}

Expand Down
Loading

0 comments on commit 151e75a

Please sign in to comment.