Skip to content

Commit 442a8c5

Browse files
committed
[Fabric] Pass children when cloning nodes
1 parent e996d90 commit 442a8c5

File tree

6 files changed

+127
-44
lines changed

6 files changed

+127
-44
lines changed

packages/react-native-renderer/src/ReactFiberConfigFabric.js

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -349,9 +349,8 @@ export function cloneInstance(
349349
type: string,
350350
oldProps: Props,
351351
newProps: Props,
352-
internalInstanceHandle: InternalInstanceHandle,
353352
keepChildren: boolean,
354-
recyclableInstance: null | Instance,
353+
newChildSet: ?ChildSet,
355354
): Instance {
356355
const viewConfig = instance.canonical.viewConfig;
357356
const updatePayload = diff(oldProps, newProps, viewConfig.validAttributes);
@@ -370,12 +369,26 @@ export function cloneInstance(
370369
return instance;
371370
}
372371
} else {
373-
if (updatePayload !== null) {
374-
clone = cloneNodeWithNewChildrenAndProps(node, updatePayload);
372+
// If passChildrenWhenCloningPersistedNodes is enabled, children will be non-null
373+
if (newChildSet != null) {
374+
if (updatePayload !== null) {
375+
clone = cloneNodeWithNewChildrenAndProps(
376+
node,
377+
newChildSet,
378+
updatePayload,
379+
);
380+
} else {
381+
clone = cloneNodeWithNewChildren(node, newChildSet);
382+
}
375383
} else {
376-
clone = cloneNodeWithNewChildren(node);
384+
if (updatePayload !== null) {
385+
clone = cloneNodeWithNewChildrenAndProps(node, updatePayload);
386+
} else {
387+
clone = cloneNodeWithNewChildren(node);
388+
}
377389
}
378390
}
391+
379392
return {
380393
node: clone,
381394
canonical: instance.canonical,
@@ -386,7 +399,6 @@ export function cloneHiddenInstance(
386399
instance: Instance,
387400
type: string,
388401
props: Props,
389-
internalInstanceHandle: InternalInstanceHandle,
390402
): Instance {
391403
const viewConfig = instance.canonical.viewConfig;
392404
const node = instance.node;
@@ -403,7 +415,6 @@ export function cloneHiddenInstance(
403415
export function cloneHiddenTextInstance(
404416
instance: Instance,
405417
text: string,
406-
internalInstanceHandle: InternalInstanceHandle,
407418
): TextInstance {
408419
throw new Error('Not yet implemented.');
409420
}

packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/InitializeNativeFabricUIManager.js

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -70,12 +70,15 @@ const RCTFabricUIManager = {
7070
children: node.children,
7171
};
7272
}),
73-
cloneNodeWithNewChildren: jest.fn(function cloneNodeWithNewChildren(node) {
73+
cloneNodeWithNewChildren: jest.fn(function cloneNodeWithNewChildren(
74+
node,
75+
children,
76+
) {
7477
return {
7578
reactTag: node.reactTag,
7679
viewName: node.viewName,
7780
props: node.props,
78-
children: [],
81+
children: children ?? [],
7982
};
8083
}),
8184
cloneNodeWithNewProps: jest.fn(function cloneNodeWithNewProps(
@@ -91,11 +94,17 @@ const RCTFabricUIManager = {
9194
}),
9295
cloneNodeWithNewChildrenAndProps: jest.fn(
9396
function cloneNodeWithNewChildrenAndProps(node, newPropsDiff) {
97+
let children = [];
98+
if (arguments.length === 3) {
99+
children = newPropsDiff;
100+
newPropsDiff = arguments[2];
101+
}
102+
94103
return {
95104
reactTag: node.reactTag,
96105
viewName: node.viewName,
97106
props: {...node.props, ...newPropsDiff},
98-
children: [],
107+
children,
99108
};
100109
},
101110
),

packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -220,6 +220,60 @@ describe('ReactFabric', () => {
220220
).toMatchSnapshot();
221221
});
222222

223+
it.each([true, false])(
224+
'should not clone nodes without children when updating props (feature flag = %p)',
225+
async passChildren => {
226+
const ReactFeatureFlags = require('shared/ReactFeatureFlags');
227+
ReactFeatureFlags.passChildrenWhenCloningPersistedNodes = passChildren;
228+
229+
const View = createReactNativeComponentClass('RCTView', () => ({
230+
validAttributes: {foo: true},
231+
uiViewClassName: 'RCTView',
232+
}));
233+
234+
const Component = ({foo}) => (
235+
<View>
236+
<View foo={foo} />
237+
</View>
238+
);
239+
240+
await act(() => ReactFabric.render(<Component foo={true} />, 11));
241+
expect(nativeFabricUIManager.completeRoot).toBeCalled();
242+
jest.clearAllMocks();
243+
244+
await act(() => ReactFabric.render(<Component foo={false} />, 11));
245+
expect(nativeFabricUIManager.cloneNode).not.toBeCalled();
246+
expect(nativeFabricUIManager.cloneNodeWithNewProps).toHaveBeenCalledTimes(
247+
1,
248+
);
249+
expect(nativeFabricUIManager.cloneNodeWithNewProps).toHaveBeenCalledWith(
250+
expect.anything(),
251+
{foo: false},
252+
);
253+
254+
expect(
255+
nativeFabricUIManager.cloneNodeWithNewChildren,
256+
).toHaveBeenCalledTimes(1);
257+
if (passChildren) {
258+
expect(
259+
nativeFabricUIManager.cloneNodeWithNewChildren,
260+
).toHaveBeenCalledWith(expect.anything(), [
261+
expect.objectContaining({props: {foo: false}}),
262+
]);
263+
expect(nativeFabricUIManager.appendChild).not.toBeCalled();
264+
} else {
265+
expect(
266+
nativeFabricUIManager.cloneNodeWithNewChildren,
267+
).toHaveBeenCalledWith(expect.anything());
268+
expect(nativeFabricUIManager.appendChild).toHaveBeenCalledTimes(1);
269+
}
270+
expect(
271+
nativeFabricUIManager.cloneNodeWithNewChildrenAndProps,
272+
).not.toBeCalled();
273+
expect(nativeFabricUIManager.completeRoot).toBeCalled();
274+
},
275+
);
276+
223277
it('should call dispatchCommand for native refs', async () => {
224278
const View = createReactNativeComponentClass('RCTView', () => ({
225279
validAttributes: {foo: true},

packages/react-noop-renderer/src/createReactNoop.js

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -223,9 +223,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
223223
type: string,
224224
oldProps: Props,
225225
newProps: Props,
226-
internalInstanceHandle: Object,
227226
keepChildren: boolean,
228-
recyclableInstance: null | Instance,
227+
children: ?$ReadOnlyArray<Instance>,
229228
): Instance {
230229
if (__DEV__) {
231230
checkPropStringCoercion(newProps.children, 'children');
@@ -234,7 +233,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
234233
id: instance.id,
235234
type: type,
236235
parent: instance.parent,
237-
children: keepChildren ? instance.children : [],
236+
children: keepChildren ? instance.children : children ?? [],
238237
text: shouldSetTextContent(type, newProps)
239238
? computeText((newProps.children: any) + '', instance.context)
240239
: null,
@@ -740,25 +739,15 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
740739
instance: Instance,
741740
type: string,
742741
props: Props,
743-
internalInstanceHandle: Object,
744742
): Instance {
745-
const clone = cloneInstance(
746-
instance,
747-
type,
748-
props,
749-
props,
750-
internalInstanceHandle,
751-
true,
752-
null,
753-
);
743+
const clone = cloneInstance(instance, type, props, props, true, null);
754744
clone.hidden = true;
755745
return clone;
756746
},
757747

758748
cloneHiddenTextInstance(
759749
instance: TextInstance,
760750
text: string,
761-
internalInstanceHandle: Object,
762751
): TextInstance {
763752
const clone = {
764753
text: instance.text,

packages/react-reconciler/src/ReactFiberCompleteWork.js

Lines changed: 31 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ import {
3838
enableCache,
3939
enableTransitionTracing,
4040
enableFloat,
41+
passChildrenWhenCloningPersistedNodes,
4142
} from 'shared/ReactFeatureFlags';
4243

4344
import {now} from './Scheduler';
@@ -253,27 +254,26 @@ function appendAllChildren(
253254
node.sibling.return = node.return;
254255
node = node.sibling;
255256
}
256-
} else if (supportsPersistence) {
257+
} else if (supportsPersistence && !passChildrenWhenCloningPersistedNodes) {
257258
// We only have the top Fiber that was created but we need recurse down its
258259
// children to find all the terminal nodes.
259260
let node = workInProgress.child;
260261
while (node !== null) {
261-
// eslint-disable-next-line no-labels
262-
branches: if (node.tag === HostComponent) {
262+
if (node.tag === HostComponent) {
263263
let instance = node.stateNode;
264264
if (needsVisibilityToggle && isHidden) {
265265
// This child is inside a timed out tree. Hide it.
266266
const props = node.memoizedProps;
267267
const type = node.type;
268-
instance = cloneHiddenInstance(instance, type, props, node);
268+
instance = cloneHiddenInstance(instance, type, props);
269269
}
270270
appendInitialChild(parent, instance);
271271
} else if (node.tag === HostText) {
272272
let instance = node.stateNode;
273273
if (needsVisibilityToggle && isHidden) {
274274
// This child is inside a timed out tree. Hide it.
275275
const text = node.memoizedProps;
276-
instance = cloneHiddenTextInstance(instance, text, node);
276+
instance = cloneHiddenTextInstance(instance, text);
277277
}
278278
appendInitialChild(parent, instance);
279279
} else if (node.tag === HostPortal) {
@@ -290,7 +290,12 @@ function appendAllChildren(
290290
if (child !== null) {
291291
child.return = node;
292292
}
293-
appendAllChildren(parent, node, true, true);
293+
appendAllChildren(
294+
parent,
295+
node,
296+
/* needsVisibilityToggle */ true,
297+
/* isHidden */ true,
298+
);
294299
} else if (node.child !== null) {
295300
node.child.return = node;
296301
node = node.child;
@@ -328,31 +333,28 @@ function appendAllChildrenToContainer(
328333
let node = workInProgress.child;
329334
while (node !== null) {
330335
// eslint-disable-next-line no-labels
331-
branches: if (node.tag === HostComponent) {
336+
if (node.tag === HostComponent) {
332337
let instance = node.stateNode;
333338
if (needsVisibilityToggle && isHidden) {
334339
// This child is inside a timed out tree. Hide it.
335340
const props = node.memoizedProps;
336341
const type = node.type;
337-
instance = cloneHiddenInstance(instance, type, props, node);
342+
instance = cloneHiddenInstance(instance, type, props);
338343
}
339344
appendChildToContainerChildSet(containerChildSet, instance);
340345
} else if (node.tag === HostText) {
341346
let instance = node.stateNode;
342347
if (needsVisibilityToggle && isHidden) {
343348
// This child is inside a timed out tree. Hide it.
344349
const text = node.memoizedProps;
345-
instance = cloneHiddenTextInstance(instance, text, node);
350+
instance = cloneHiddenTextInstance(instance, text);
346351
}
347352
appendChildToContainerChildSet(containerChildSet, instance);
348353
} else if (node.tag === HostPortal) {
349354
// If we have a portal child, then we don't want to traverse
350355
// down its children. Instead, we'll get insertions from each child in
351356
// the portal directly.
352-
} else if (
353-
node.tag === OffscreenComponent &&
354-
node.memoizedState !== null
355-
) {
357+
} else if (node.tag === OffscreenComponent && node.memoizedState !== null) {
356358
// The children in this boundary are hidden. Toggle their visibility
357359
// before appending.
358360
const child = node.child;
@@ -364,8 +366,8 @@ function appendAllChildrenToContainer(
364366
appendAllChildrenToContainer(
365367
containerChildSet,
366368
node,
367-
_needsVisibilityToggle,
368-
true,
369+
/* needsVisibilityToggle */ _needsVisibilityToggle,
370+
/* isHidden */ true,
369371
);
370372
} else if (node.child !== null) {
371373
node.child.return = node;
@@ -449,16 +451,27 @@ function updateHostComponent(
449451
workInProgress.stateNode = currentInstance;
450452
return;
451453
}
452-
const recyclableInstance: Instance = workInProgress.stateNode;
453454
const currentHostContext = getHostContext();
455+
456+
let newChildSet = null;
457+
if (!childrenUnchanged && passChildrenWhenCloningPersistedNodes) {
458+
newChildSet = createContainerChildSet();
459+
// If children might have changed, we have to add them all to the set.
460+
appendAllChildrenToContainer(
461+
newChildSet,
462+
workInProgress,
463+
/* needsVisibilityToggle */ false,
464+
/* isHidden */ false,
465+
);
466+
}
467+
454468
const newInstance = cloneInstance(
455469
currentInstance,
456470
type,
457471
oldProps,
458472
newProps,
459-
workInProgress,
460473
childrenUnchanged,
461-
recyclableInstance,
474+
newChildSet,
462475
);
463476
if (newInstance === currentInstance) {
464477
// No changes, just reuse the existing instance.

scripts/flow/react-native-host-hooks.js

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -176,9 +176,16 @@ declare var nativeFabricUIManager: {
176176
eventTarget: Object,
177177
) => Object,
178178
cloneNode: (node: Object) => Object,
179-
cloneNodeWithNewChildren: (node: Object) => Object,
179+
cloneNodeWithNewChildren: (
180+
node: Object,
181+
children?: $ReadOnlyArray<Object>,
182+
) => Object,
180183
cloneNodeWithNewProps: (node: Object, newProps: ?Object) => Object,
181-
cloneNodeWithNewChildrenAndProps: (node: Object, newProps: ?Object) => Object,
184+
cloneNodeWithNewChildrenAndProps: (
185+
node: Object,
186+
newPropsOrChildren: ?Object | $ReadOnlyArray<Object>,
187+
newProps?: ?Object,
188+
) => Object,
182189
appendChild: (node: Object, childNode: Object) => void,
183190

184191
createChildSet: () => Object,

0 commit comments

Comments
 (0)