Skip to content

Commit fe94a4e

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

File tree

7 files changed

+128
-42
lines changed

7 files changed

+128
-42
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 & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -210,8 +210,13 @@ describe('ReactFabric', () => {
210210
11,
211211
);
212212
});
213+
const argIndex = gate(flags => flags.passChildrenWhenCloningPersistedNodes)
214+
? 2
215+
: 1;
213216
expect(
214-
nativeFabricUIManager.cloneNodeWithNewChildrenAndProps.mock.calls[0][1],
217+
nativeFabricUIManager.cloneNodeWithNewChildrenAndProps.mock.calls[0][
218+
argIndex
219+
],
215220
).toEqual({
216221
foo: 'b',
217222
});
@@ -220,6 +225,54 @@ describe('ReactFabric', () => {
220225
).toMatchSnapshot();
221226
});
222227

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+
223276
it('should call dispatchCommand for native refs', async () => {
224277
const View = createReactNativeComponentClass('RCTView', () => ({
225278
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 & 14 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';
@@ -258,22 +259,21 @@ function appendAllChildren(
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,21 +333,21 @@ 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) {
@@ -364,8 +369,8 @@ function appendAllChildrenToContainer(
364369
appendAllChildrenToContainer(
365370
containerChildSet,
366371
node,
367-
_needsVisibilityToggle,
368-
true,
372+
/* needsVisibilityToggle */ _needsVisibilityToggle,
373+
/* isHidden */ true,
369374
);
370375
} else if (node.child !== null) {
371376
node.child.return = node;
@@ -449,16 +454,27 @@ function updateHostComponent(
449454
workInProgress.stateNode = currentInstance;
450455
return;
451456
}
452-
const recyclableInstance: Instance = workInProgress.stateNode;
453457
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+
454471
const newInstance = cloneInstance(
455472
currentInstance,
456473
type,
457474
oldProps,
458475
newProps,
459-
workInProgress,
460476
childrenUnchanged,
461-
recyclableInstance,
477+
newChildSet,
462478
);
463479
if (newInstance === currentInstance) {
464480
// No changes, just reuse the existing instance.
@@ -481,7 +497,7 @@ function updateHostComponent(
481497
// Even though we're not going to use it for anything.
482498
// Otherwise parents won't know that there are new children to propagate upwards.
483499
markUpdate(workInProgress);
484-
} else {
500+
} else if (!passChildrenWhenCloningPersistedNodes) {
485501
// If children might have changed, we have to add them all to the set.
486502
appendAllChildren(
487503
newInstance,
@@ -1274,6 +1290,7 @@ function completeWork(
12741290
currentHostContext,
12751291
workInProgress,
12761292
);
1293+
// TODO: pass children as part of the initial instance creation
12771294
appendAllChildren(instance, workInProgress, false, false);
12781295
workInProgress.stateNode = instance;
12791296

packages/shared/forks/ReactFeatureFlags.native-oss.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ export const enableAsyncActions = false;
7979
export const alwaysThrottleRetries = true;
8080

8181
export const useMicrotasksForSchedulingInFabric = false;
82-
export const passChildrenWhenCloningPersistedNodes = false;
82+
export const passChildrenWhenCloningPersistedNodes = __VARIANT__;
8383

8484
// Flow magic to verify the exports of this file match the original version.
8585
((((null: any): ExportsType): FeatureFlagsType): ExportsType);

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)