[Fabric] Pass children when cloning (#27458)
## 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: https://github.com/facebook/react-native/pull/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.
Pieter De Baets committed
Oct 10, 2023 at 15:11 UTC
151e75a128d0fd436dce365335b96c5686f704d4
16 files changed
+178
-55
packages/react-native-renderer/src/ReactFiberConfigFabric.js
+37
-13
@@ -47,7 +47,10 @@ const {
47
unstable_getCurrentEventPriority: fabricGetCurrentEventPriority,
48
} = nativeFabricUIManager;
49
50
-import {useMicrotasksForSchedulingInFabric} from 'shared/ReactFeatureFlags';
50
+import {
51
+ useMicrotasksForSchedulingInFabric,
52
+ passChildrenWhenCloningPersistedNodes,
53
+} from 'shared/ReactFeatureFlags';
54
55
const {get: getViewConfigForType} = ReactNativeViewConfigRegistry;
56
@@ -87,7 +90,7 @@ export type TextInstance = {
90
export type HydratableInstance = Instance | TextInstance;
91
export type PublicInstance = ReactNativePublicInstance;
92
export type Container = number;
90
-export type ChildSet = Object;
93
+export type ChildSet = Object | Array<Node>;
94
export type HostContext = $ReadOnly<{
95
isInAParentText: boolean,
96
}>;
@@ -346,9 +349,8 @@ export function cloneInstance(
349
type: string,
350
oldProps: Props,
351
newProps: Props,
349
- internalInstanceHandle: InternalInstanceHandle,
352
keepChildren: boolean,
351
- recyclableInstance: null | Instance,
353
+ newChildSet: ?ChildSet,
354
): Instance {
355
const viewConfig = instance.canonical.viewConfig;
356
const updatePayload = diff(oldProps, newProps, viewConfig.validAttributes);
@@ -367,12 +369,26 @@ export function cloneInstance(
369
return instance;
370
}
371
} else {
370
- if (updatePayload !== null) {
371
- 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
+ }
383
} else {
373
- clone = cloneNodeWithNewChildren(node);
384
+ if (updatePayload !== null) {
385
+ clone = cloneNodeWithNewChildrenAndProps(node, updatePayload);
386
+ } else {
387
+ clone = cloneNodeWithNewChildren(node);
388
+ }
389
}
390
}
391
+
392
return {
393
node: clone,
394
canonical: instance.canonical,
@@ -383,7 +399,6 @@ export function cloneHiddenInstance(
399
instance: Instance,
400
type: string,
401
props: Props,
386
- internalInstanceHandle: InternalInstanceHandle,
402
): Instance {
403
const viewConfig = instance.canonical.viewConfig;
404
const node = instance.node;
@@ -400,20 +415,27 @@ export function cloneHiddenInstance(
415
export function cloneHiddenTextInstance(
416
instance: Instance,
417
text: string,
403
- internalInstanceHandle: InternalInstanceHandle,
418
): TextInstance {
419
throw new Error('Not yet implemented.');
420
}
421
408
-export function createContainerChildSet(container: Container): ChildSet {
409
- return createChildNodeSet(container);
422
+export function createContainerChildSet(): ChildSet {
423
+ if (passChildrenWhenCloningPersistedNodes) {
424
+ return [];
425
+ } else {
426
+ return createChildNodeSet();
427
+ }
428
}
429
430
export function appendChildToContainerChildSet(
431
childSet: ChildSet,
432
child: Instance | TextInstance,
433
): void {
416
- appendChildNodeToSet(childSet, child.node);
434
+ if (passChildrenWhenCloningPersistedNodes) {
435
+ childSet.push(child.node);
436
+ } else {
437
+ appendChildNodeToSet(childSet, child.node);
438
+ }
439
}
440
441
export function finalizeContainerChildren(
@@ -426,7 +448,9 @@ export function finalizeContainerChildren(
448
export function replaceContainerChildren(
449
container: Container,
450
newChildren: ChildSet,
429
-): void {}
451
+): void {
452
+ // Noop - children will be replaced in finalizeContainerChildren
453
+}
454
455
export function getInstanceFromNode(node: any): empty {
456
throw new Error('Not yet implemented.');
packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/InitializeNativeFabricUIManager.js
+12
-3
@@ -70,12 +70,15 @@ const RCTFabricUIManager = {
70
children: node.children,
71
};
72
}),
73
- cloneNodeWithNewChildren: jest.fn(function cloneNodeWithNewChildren(node) {
73
+ cloneNodeWithNewChildren: jest.fn(function cloneNodeWithNewChildren(
74
+ node,
75
+ children,
76
+ ) {
77
return {
78
reactTag: node.reactTag,
79
viewName: node.viewName,
80
props: node.props,
78
- children: [],
81
+ children: children ?? [],
82
};
83
}),
84
cloneNodeWithNewProps: jest.fn(function cloneNodeWithNewProps(
@@ -91,11 +94,17 @@ const RCTFabricUIManager = {
94
}),
95
cloneNodeWithNewChildrenAndProps: jest.fn(
96
function cloneNodeWithNewChildrenAndProps(node, newPropsDiff) {
97
+ let children = [];
98
+ if (arguments.length === 3) {
99
+ children = newPropsDiff;
100
+ newPropsDiff = arguments[2];
101
+ }
102
+
103
return {
104
reactTag: node.reactTag,
105
viewName: node.viewName,
106
props: {...node.props, ...newPropsDiff},
98
- children: [],
107
+ children,
108
};
109
},
110
),
packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js
+54
-1
@@ -210,8 +210,13 @@ describe('ReactFabric', () => {
210
11,
211
);
212
});
213
+ const argIndex = gate(flags => flags.passChildrenWhenCloningPersistedNodes)
214
+ ? 2
215
+ : 1;
216
expect(
214
- nativeFabricUIManager.cloneNodeWithNewChildrenAndProps.mock.calls[0][1],
217
+ nativeFabricUIManager.cloneNodeWithNewChildrenAndProps.mock.calls[0][
218
+ argIndex
219
+ ],
220
).toEqual({
221
foo: 'b',
222
});
@@ -220,6 +225,54 @@ describe('ReactFabric', () => {
225
).toMatchSnapshot();
226
});
227
228
+ it('should not clone nodes without children when updating props', async () => {
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 (gate(flags => flags.passChildrenWhenCloningPersistedNodes)) {
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
it('should call dispatchCommand for native refs', async () => {
277
const View = createReactNativeComponentClass('RCTView', () => ({
278
validAttributes: {foo: true},
packages/react-noop-renderer/src/createReactNoop.js
+4
-17
@@ -223,9 +223,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
223
type: string,
224
oldProps: Props,
225
newProps: Props,
226
- internalInstanceHandle: Object,
226
keepChildren: boolean,
228
- recyclableInstance: null | Instance,
227
+ children: ?$ReadOnlyArray<Instance>,
228
): Instance {
229
if (__DEV__) {
230
checkPropStringCoercion(newProps.children, 'children');
@@ -234,7 +233,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
233
id: instance.id,
234
type: type,
235
parent: instance.parent,
237
- children: keepChildren ? instance.children : [],
236
+ children: keepChildren ? instance.children : children ?? [],
237
text: shouldSetTextContent(type, newProps)
238
? computeText((newProps.children: any) + '', instance.context)
239
: null,
@@ -704,9 +703,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
703
cloneInstance,
704
clearContainer,
705
707
- createContainerChildSet(
708
- container: Container,
709
- ): Array<Instance | TextInstance> {
706
+ createContainerChildSet(): Array<Instance | TextInstance> {
707
return [];
708
},
709
@@ -742,17 +739,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
739
instance: Instance,
740
type: string,
741
props: Props,
745
- internalInstanceHandle: Object,
742
): Instance {
747
- const clone = cloneInstance(
748
- instance,
749
- type,
750
- props,
751
- props,
752
- internalInstanceHandle,
753
- true,
754
- null,
755
- );
743
+ const clone = cloneInstance(instance, type, props, props, true, null);
744
clone.hidden = true;
745
return clone;
746
},
@@ -760,7 +748,6 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
748
cloneHiddenTextInstance(
749
instance: TextInstance,
750
text: string,
763
- internalInstanceHandle: Object,
751
): TextInstance {
752
const clone = {
753
text: instance.text,
packages/react-reconciler/src/ReactFiberCommitWork.js
+1
-1
@@ -1724,7 +1724,7 @@ function emptyPortalContainer(current: Fiber) {
1724
...
1725
} = current.stateNode;
1726
const {containerInfo} = portal;
1727
- const emptyChildSet = createContainerChildSet(containerInfo);
1727
+ const emptyChildSet = createContainerChildSet();
1728
replaceContainerChildren(containerInfo, emptyChildSet);
1729
}
1730
packages/react-reconciler/src/ReactFiberCompleteWork.js
+50
-17
@@ -38,6 +38,7 @@ import {
38
enableCache,
39
enableTransitionTracing,
40
enableFloat,
41
+ passChildrenWhenCloningPersistedNodes,
42
} from 'shared/ReactFeatureFlags';
43
44
import {now} from './Scheduler';
@@ -258,14 +259,13 @@ function appendAllChildren(
259
// children to find all the terminal nodes.
260
let node = workInProgress.child;
261
while (node !== null) {
261
- // eslint-disable-next-line no-labels
262
- branches: if (node.tag === HostComponent) {
262
+ if (node.tag === HostComponent) {
263
let instance = node.stateNode;
264
if (needsVisibilityToggle && isHidden) {
265
// This child is inside a timed out tree. Hide it.
266
const props = node.memoizedProps;
267
const type = node.type;
268
- instance = cloneHiddenInstance(instance, type, props, node);
268
+ instance = cloneHiddenInstance(instance, type, props);
269
}
270
appendInitialChild(parent, instance);
271
} else if (node.tag === HostText) {
@@ -273,7 +273,7 @@ function appendAllChildren(
273
if (needsVisibilityToggle && isHidden) {
274
// This child is inside a timed out tree. Hide it.
275
const text = node.memoizedProps;
276
- instance = cloneHiddenTextInstance(instance, text, node);
276
+ instance = cloneHiddenTextInstance(instance, text);
277
}
278
appendInitialChild(parent, instance);
279
} else if (node.tag === HostPortal) {
@@ -290,7 +290,12 @@ function appendAllChildren(
290
if (child !== null) {
291
child.return = node;
292
}
293
- appendAllChildren(parent, node, true, true);
293
+ appendAllChildren(
294
+ parent,
295
+ node,
296
+ /* needsVisibilityToggle */ true,
297
+ /* isHidden */ true,
298
+ );
299
} else if (node.child !== null) {
300
node.child.return = node;
301
node = node.child;
@@ -328,13 +333,13 @@ function appendAllChildrenToContainer(
333
let node = workInProgress.child;
334
while (node !== null) {
335
// eslint-disable-next-line no-labels
331
- branches: if (node.tag === HostComponent) {
336
+ if (node.tag === HostComponent) {
337
let instance = node.stateNode;
338
if (needsVisibilityToggle && isHidden) {
339
// This child is inside a timed out tree. Hide it.
340
const props = node.memoizedProps;
341
const type = node.type;
337
- instance = cloneHiddenInstance(instance, type, props, node);
342
+ instance = cloneHiddenInstance(instance, type, props);
343
}
344
appendChildToContainerChildSet(containerChildSet, instance);
345
} else if (node.tag === HostText) {
@@ -342,7 +347,7 @@ function appendAllChildrenToContainer(
347
if (needsVisibilityToggle && isHidden) {
348
// This child is inside a timed out tree. Hide it.
349
const text = node.memoizedProps;
345
- instance = cloneHiddenTextInstance(instance, text, node);
350
+ instance = cloneHiddenTextInstance(instance, text);
351
}
352
appendChildToContainerChildSet(containerChildSet, instance);
353
} else if (node.tag === HostPortal) {
@@ -364,8 +369,8 @@ function appendAllChildrenToContainer(
369
appendAllChildrenToContainer(
370
containerChildSet,
371
node,
367
- _needsVisibilityToggle,
368
- true,
372
+ /* needsVisibilityToggle */ _needsVisibilityToggle,
373
+ /* isHidden */ true,
374
);
375
} else if (node.child !== null) {
376
node.child.return = node;
@@ -390,6 +395,7 @@ function appendAllChildrenToContainer(
395
}
396
}
397
}
398
+
399
function updateHostContainer(current: null | Fiber, workInProgress: Fiber) {
400
if (supportsPersistence) {
401
const portalOrRoot: {
@@ -402,9 +408,14 @@ function updateHostContainer(current: null | Fiber, workInProgress: Fiber) {
408
// No changes, just reuse the existing instance.
409
} else {
410
const container = portalOrRoot.containerInfo;
405
- const newChildSet = createContainerChildSet(container);
411
+ const newChildSet = createContainerChildSet();
412
// If children might have changed, we have to add them all to the set.
407
- appendAllChildrenToContainer(newChildSet, workInProgress, false, false);
413
+ appendAllChildrenToContainer(
414
+ newChildSet,
415
+ workInProgress,
416
+ /* needsVisibilityToggle */ false,
417
+ /* isHidden */ false,
418
+ );
419
portalOrRoot.pendingChildren = newChildSet;
420
// Schedule an update on the container to swap out the container.
421
markUpdate(workInProgress);
@@ -412,6 +423,7 @@ function updateHostContainer(current: null | Fiber, workInProgress: Fiber) {
423
}
424
}
425
}
426
+
427
function updateHostComponent(
428
current: Fiber,
429
workInProgress: Fiber,
@@ -442,16 +454,27 @@ function updateHostComponent(
454
workInProgress.stateNode = currentInstance;
455
return;
456
}
445
- const recyclableInstance: Instance = workInProgress.stateNode;
457
const currentHostContext = getHostContext();
458
+
459
+ let newChildSet = null;
460
+ if (!childrenUnchanged && passChildrenWhenCloningPersistedNodes) {
461
+ newChildSet = createContainerChildSet();
462
+ // If children might have changed, we have to add them all to the set.
463
+ appendAllChildrenToContainer(
464
+ newChildSet,
465
+ workInProgress,
466
+ /* needsVisibilityToggle */ false,
467
+ /* isHidden */ false,
468
+ );
469
+ }
470
+
471
const newInstance = cloneInstance(
472
currentInstance,
473
type,
474
oldProps,
475
newProps,
452
- workInProgress,
476
childrenUnchanged,
454
- recyclableInstance,
477
+ newChildSet,
478
);
479
if (newInstance === currentInstance) {
480
// No changes, just reuse the existing instance.
@@ -460,6 +483,9 @@ function updateHostComponent(
483
return;
484
}
485
486
+ // Certain renderers require commit-time effects for initial mount.
487
+ // (eg DOM renderer supports auto-focus for certain elements).
488
+ // Make sure such renderers get scheduled for later work.
489
if (
490
finalizeInitialChildren(newInstance, type, newProps, currentHostContext)
491
) {
@@ -471,9 +497,14 @@ function updateHostComponent(
497
// Even though we're not going to use it for anything.
498
// Otherwise parents won't know that there are new children to propagate upwards.
499
markUpdate(workInProgress);
474
- } else {
500
+ } else if (!passChildrenWhenCloningPersistedNodes) {
501
// If children might have changed, we have to add them all to the set.
476
- appendAllChildren(newInstance, workInProgress, false, false);
502
+ appendAllChildren(
503
+ newInstance,
504
+ workInProgress,
505
+ /* needsVisibilityToggle */ false,
506
+ /* isHidden */ false,
507
+ );
508
}
509
}
510
}
@@ -1259,6 +1290,8 @@ function completeWork(
1290
currentHostContext,
1291
workInProgress,
1292
);
1293
+ // TODO: For persistent renderers, we should pass children as part
1294
+ // of the initial instance creation
1295
appendAllChildren(instance, workInProgress, false, false);
1296
workInProgress.stateNode = instance;
1297
packages/shared/ReactFeatureFlags.js
+2
@@ -124,6 +124,8 @@ export const alwaysThrottleRetries = true;
124
125
export const useMicrotasksForSchedulingInFabric = false;
126
127
+export const passChildrenWhenCloningPersistedNodes = false;
128
+
129
// -----------------------------------------------------------------------------
130
// Chopping Block
131
//
packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js
+1
@@ -24,6 +24,7 @@ export const enableUseRefAccessWarning = __VARIANT__;
24
export const enableDeferRootSchedulingToMicrotask = __VARIANT__;
25
export const alwaysThrottleRetries = __VARIANT__;
26
export const useMicrotasksForSchedulingInFabric = __VARIANT__;
27
+export const passChildrenWhenCloningPersistedNodes = __VARIANT__;
28
29
// Flow magic to verify the exports of this file match the original version.
30
((((null: any): ExportsType): DynamicFlagsType): ExportsType);
packages/shared/forks/ReactFeatureFlags.native-fb.js
+1
@@ -22,6 +22,7 @@ export const {
22
enableDeferRootSchedulingToMicrotask,
23
alwaysThrottleRetries,
24
useMicrotasksForSchedulingInFabric,
25
+ passChildrenWhenCloningPersistedNodes,
26
} = dynamicFlags;
27
28
// The rest of the flags are static for better dead code elimination.
packages/shared/forks/ReactFeatureFlags.native-oss.js
+1
@@ -79,6 +79,7 @@ export const enableAsyncActions = false;
79
export const alwaysThrottleRetries = true;
80
81
export const useMicrotasksForSchedulingInFabric = false;
82
+export const passChildrenWhenCloningPersistedNodes = false;
83
84
// Flow magic to verify the exports of this file match the original version.
85
((((null: any): ExportsType): FeatureFlagsType): ExportsType);
packages/shared/forks/ReactFeatureFlags.test-renderer.js
+1
@@ -79,6 +79,7 @@ export const enableAsyncActions = true;
79
export const alwaysThrottleRetries = true;
80
81
export const useMicrotasksForSchedulingInFabric = false;
82
+export const passChildrenWhenCloningPersistedNodes = false;
83
84
// Flow magic to verify the exports of this file match the original version.
85
((((null: any): ExportsType): FeatureFlagsType): ExportsType);
packages/shared/forks/ReactFeatureFlags.test-renderer.native.js
+1
@@ -76,6 +76,7 @@ export const enableAsyncActions = true;
76
export const alwaysThrottleRetries = true;
77
78
export const useMicrotasksForSchedulingInFabric = false;
79
+export const passChildrenWhenCloningPersistedNodes = false;
80
81
// Flow magic to verify the exports of this file match the original version.
82
((((null: any): ExportsType): FeatureFlagsType): ExportsType);
packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
+1
@@ -79,6 +79,7 @@ export const enableAsyncActions = true;
79
export const alwaysThrottleRetries = true;
80
81
export const useMicrotasksForSchedulingInFabric = false;
82
+export const passChildrenWhenCloningPersistedNodes = false;
83
84
// Flow magic to verify the exports of this file match the original version.
85
((((null: any): ExportsType): FeatureFlagsType): ExportsType);
packages/shared/forks/ReactFeatureFlags.www.js
+1
@@ -110,6 +110,7 @@ export const enableFizzExternalRuntime = true;
110
export const forceConcurrentByDefaultForTesting = false;
111
112
export const useMicrotasksForSchedulingInFabric = false;
113
+export const passChildrenWhenCloningPersistedNodes = false;
114
115
// Flow magic to verify the exports of this file match the original version.
116
((((null: any): ExportsType): FeatureFlagsType): ExportsType);
scripts/flow/react-native-host-hooks.js
+10
-3
@@ -176,12 +176,19 @@ declare var nativeFabricUIManager: {
176
eventTarget: Object,
177
) => Object,
178
cloneNode: (node: Object) => Object,
179
- cloneNodeWithNewChildren: (node: Object) => Object,
179
+ cloneNodeWithNewChildren: (
180
+ node: Object,
181
+ children?: $ReadOnlyArray<Object>,
182
+ ) => Object,
183
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,
189
appendChild: (node: Object, childNode: Object) => void,
190
184
- createChildSet: (rootTag: number) => Object,
191
+ createChildSet: () => Object,
192
appendChildToSet: (childSet: Object, childNode: Object) => void,
193
completeRoot: (rootTag: number, childSet: Object) => void,
194
registerEventHandler: (
scripts/flow/xplat.js
+1
@@ -12,4 +12,5 @@ declare module 'ReactNativeInternalFeatureFlags' {
12
declare export var enableDeferRootSchedulingToMicrotask: boolean;
13
declare export var alwaysThrottleRetries: boolean;
14
declare export var useMicrotasksForSchedulingInFabric: boolean;
15
+ declare export var passChildrenWhenCloningPersistedNodes: boolean;
16
}