Skip to content

Commit f260b14

Browse files
gaearongaearon
authored
Fix host bailout for the persistent mode (facebook#13611)
* Add regression test for persistent bailout bug * Fork more logic into updateHostComponent This is mostly copy paste. But I added a bailout only to mutation mode. Persistent mode doesn't have that props equality bailout anymore, so the Fabric test now passes. * Add failing test for persistence host minimalism * Add bailouts to the persistent host updates
1 parent 4a40d76 commit f260b14

File tree

5 files changed

+353
-83
lines changed

5 files changed

+353
-83
lines changed

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

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,45 @@ describe('ReactFabric', () => {
237237
expect(FabricUIManager.__dumpHierarchyForJestTestsOnly()).toMatchSnapshot();
238238
});
239239

240+
it('recreates host parents even if only children changed', () => {
241+
const View = createReactNativeComponentClass('RCTView', () => ({
242+
validAttributes: {title: true},
243+
uiViewClassName: 'RCTView',
244+
}));
245+
246+
const before = 'abcdefghijklmnopqrst';
247+
const after = 'mxhpgwfralkeoivcstzy';
248+
249+
class Component extends React.Component {
250+
state = {
251+
chars: before,
252+
};
253+
render() {
254+
const chars = this.state.chars.split('');
255+
return (
256+
<View>{chars.map(text => <View key={text} title={text} />)}</View>
257+
);
258+
}
259+
}
260+
261+
const ref = React.createRef();
262+
// Wrap in a host node.
263+
ReactFabric.render(
264+
<View>
265+
<Component ref={ref} />
266+
</View>,
267+
11,
268+
);
269+
expect(FabricUIManager.__dumpHierarchyForJestTestsOnly()).toMatchSnapshot();
270+
271+
// Call setState() so that we skip over the top-level host node.
272+
// It should still get recreated despite a bailout.
273+
ref.current.setState({
274+
chars: after,
275+
});
276+
expect(FabricUIManager.__dumpHierarchyForJestTestsOnly()).toMatchSnapshot();
277+
});
278+
240279
it('calls setState with no arguments', () => {
241280
let mockArgs;
242281
class Component extends React.Component {

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

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,57 @@
11
// Jest Snapshot v1, https://goo.gl/fbAQLP
22

3+
exports[`ReactFabric recreates host parents even if only children changed 1`] = `
4+
"11
5+
RCTView null
6+
RCTView null
7+
RCTView {\\"title\\":\\"a\\"}
8+
RCTView {\\"title\\":\\"b\\"}
9+
RCTView {\\"title\\":\\"c\\"}
10+
RCTView {\\"title\\":\\"d\\"}
11+
RCTView {\\"title\\":\\"e\\"}
12+
RCTView {\\"title\\":\\"f\\"}
13+
RCTView {\\"title\\":\\"g\\"}
14+
RCTView {\\"title\\":\\"h\\"}
15+
RCTView {\\"title\\":\\"i\\"}
16+
RCTView {\\"title\\":\\"j\\"}
17+
RCTView {\\"title\\":\\"k\\"}
18+
RCTView {\\"title\\":\\"l\\"}
19+
RCTView {\\"title\\":\\"m\\"}
20+
RCTView {\\"title\\":\\"n\\"}
21+
RCTView {\\"title\\":\\"o\\"}
22+
RCTView {\\"title\\":\\"p\\"}
23+
RCTView {\\"title\\":\\"q\\"}
24+
RCTView {\\"title\\":\\"r\\"}
25+
RCTView {\\"title\\":\\"s\\"}
26+
RCTView {\\"title\\":\\"t\\"}"
27+
`;
28+
29+
exports[`ReactFabric recreates host parents even if only children changed 2`] = `
30+
"11
31+
RCTView null
32+
RCTView null
33+
RCTView {\\"title\\":\\"m\\"}
34+
RCTView {\\"title\\":\\"x\\"}
35+
RCTView {\\"title\\":\\"h\\"}
36+
RCTView {\\"title\\":\\"p\\"}
37+
RCTView {\\"title\\":\\"g\\"}
38+
RCTView {\\"title\\":\\"w\\"}
39+
RCTView {\\"title\\":\\"f\\"}
40+
RCTView {\\"title\\":\\"r\\"}
41+
RCTView {\\"title\\":\\"a\\"}
42+
RCTView {\\"title\\":\\"l\\"}
43+
RCTView {\\"title\\":\\"k\\"}
44+
RCTView {\\"title\\":\\"e\\"}
45+
RCTView {\\"title\\":\\"o\\"}
46+
RCTView {\\"title\\":\\"i\\"}
47+
RCTView {\\"title\\":\\"v\\"}
48+
RCTView {\\"title\\":\\"c\\"}
49+
RCTView {\\"title\\":\\"s\\"}
50+
RCTView {\\"title\\":\\"t\\"}
51+
RCTView {\\"title\\":\\"z\\"}
52+
RCTView {\\"title\\":\\"y\\"}"
53+
`;
54+
355
exports[`ReactFabric renders and reorders children 1`] = `
456
"11
557
RCTView null

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

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
4646
let instanceCounter = 0;
4747
let hostDiffCounter = 0;
4848
let hostUpdateCounter = 0;
49+
let hostCloneCounter = 0;
4950

5051
function appendChildToContainerOrInstance(
5152
parentInstance: Container | Instance,
@@ -370,6 +371,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
370371
value: clone.id,
371372
enumerable: false,
372373
});
374+
hostCloneCounter++;
373375
return clone;
374376
},
375377

@@ -579,21 +581,33 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
579581

580582
flushWithHostCounters(
581583
fn: () => void,
582-
): {|
583-
hostDiffCounter: number,
584-
hostUpdateCounter: number,
585-
|} {
584+
):
585+
| {|
586+
hostDiffCounter: number,
587+
hostUpdateCounter: number,
588+
|}
589+
| {|
590+
hostDiffCounter: number,
591+
hostCloneCounter: number,
592+
|} {
586593
hostDiffCounter = 0;
587594
hostUpdateCounter = 0;
595+
hostCloneCounter = 0;
588596
try {
589597
ReactNoop.flush();
590-
return {
591-
hostDiffCounter,
592-
hostUpdateCounter,
593-
};
598+
return useMutation
599+
? {
600+
hostDiffCounter,
601+
hostUpdateCounter,
602+
}
603+
: {
604+
hostDiffCounter,
605+
hostCloneCounter,
606+
};
594607
} finally {
595608
hostDiffCounter = 0;
596609
hostUpdateCounter = 0;
610+
hostCloneCounter = 0;
597611
}
598612
},
599613

‎packages/react-reconciler/src/ReactFiberCompleteWork.js‎

Lines changed: 83 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,8 @@ import type {
1414
Instance,
1515
Type,
1616
Props,
17-
UpdatePayload,
1817
Container,
1918
ChildSet,
20-
HostContext,
2119
} from './ReactFiberHostConfig';
2220

2321
import {
@@ -126,13 +124,36 @@ if (supportsMutation) {
126124
updateHostComponent = function(
127125
current: Fiber,
128126
workInProgress: Fiber,
129-
updatePayload: null | UpdatePayload,
130127
type: Type,
131-
oldProps: Props,
132128
newProps: Props,
133129
rootContainerInstance: Container,
134-
currentHostContext: HostContext,
135130
) {
131+
// If we have an alternate, that means this is an update and we need to
132+
// schedule a side-effect to do the updates.
133+
const oldProps = current.memoizedProps;
134+
if (oldProps === newProps) {
135+
// In mutation mode, this is sufficient for a bailout because
136+
// we won't touch this node even if children changed.
137+
return;
138+
}
139+
140+
// If we get updated because one of our children updated, we don't
141+
// have newProps so we'll have to reuse them.
142+
// TODO: Split the update API as separate for the props vs. children.
143+
// Even better would be if children weren't special cased at all tho.
144+
const instance: Instance = workInProgress.stateNode;
145+
const currentHostContext = getHostContext();
146+
// TODO: Experiencing an error where oldProps is null. Suggests a host
147+
// component is hitting the resume path. Figure out why. Possibly
148+
// related to `hidden`.
149+
const updatePayload = prepareUpdate(
150+
instance,
151+
type,
152+
oldProps,
153+
newProps,
154+
rootContainerInstance,
155+
currentHostContext,
156+
);
136157
// TODO: Type this specific to this type of component.
137158
workInProgress.updateQueue = (updatePayload: any);
138159
// If the update payload indicates that there is a change or if there
@@ -211,54 +232,70 @@ if (supportsMutation) {
211232
updateHostComponent = function(
212233
current: Fiber,
213234
workInProgress: Fiber,
214-
updatePayload: null | UpdatePayload,
215235
type: Type,
216-
oldProps: Props,
217236
newProps: Props,
218237
rootContainerInstance: Container,
219-
currentHostContext: HostContext,
220238
) {
239+
const currentInstance = current.stateNode;
240+
const oldProps = current.memoizedProps;
221241
// If there are no effects associated with this node, then none of our children had any updates.
222242
// This guarantees that we can reuse all of them.
223243
const childrenUnchanged = workInProgress.firstEffect === null;
224-
const currentInstance = current.stateNode;
225-
if (childrenUnchanged && updatePayload === null) {
244+
if (childrenUnchanged && oldProps === newProps) {
226245
// No changes, just reuse the existing instance.
227246
// Note that this might release a previous clone.
228247
workInProgress.stateNode = currentInstance;
229-
} else {
230-
let recyclableInstance = workInProgress.stateNode;
231-
let newInstance = cloneInstance(
232-
currentInstance,
233-
updatePayload,
248+
return;
249+
}
250+
const recyclableInstance: Instance = workInProgress.stateNode;
251+
const currentHostContext = getHostContext();
252+
let updatePayload = null;
253+
if (oldProps !== newProps) {
254+
updatePayload = prepareUpdate(
255+
recyclableInstance,
234256
type,
235257
oldProps,
236258
newProps,
237-
workInProgress,
238-
childrenUnchanged,
239-
recyclableInstance,
259+
rootContainerInstance,
260+
currentHostContext,
240261
);
241-
if (
242-
finalizeInitialChildren(
243-
newInstance,
244-
type,
245-
newProps,
246-
rootContainerInstance,
247-
currentHostContext,
248-
)
249-
) {
250-
markUpdate(workInProgress);
251-
}
252-
workInProgress.stateNode = newInstance;
253-
if (childrenUnchanged) {
254-
// If there are no other effects in this tree, we need to flag this node as having one.
255-
// Even though we're not going to use it for anything.
256-
// Otherwise parents won't know that there are new children to propagate upwards.
257-
markUpdate(workInProgress);
258-
} else {
259-
// If children might have changed, we have to add them all to the set.
260-
appendAllChildren(newInstance, workInProgress);
261-
}
262+
}
263+
if (childrenUnchanged && updatePayload === null) {
264+
// No changes, just reuse the existing instance.
265+
// Note that this might release a previous clone.
266+
workInProgress.stateNode = currentInstance;
267+
return;
268+
}
269+
let newInstance = cloneInstance(
270+
currentInstance,
271+
updatePayload,
272+
type,
273+
oldProps,
274+
newProps,
275+
workInProgress,
276+
childrenUnchanged,
277+
recyclableInstance,
278+
);
279+
if (
280+
finalizeInitialChildren(
281+
newInstance,
282+
type,
283+
newProps,
284+
rootContainerInstance,
285+
currentHostContext,
286+
)
287+
) {
288+
markUpdate(workInProgress);
289+
}
290+
workInProgress.stateNode = newInstance;
291+
if (childrenUnchanged) {
292+
// If there are no other effects in this tree, we need to flag this node as having one.
293+
// Even though we're not going to use it for anything.
294+
// Otherwise parents won't know that there are new children to propagate upwards.
295+
markUpdate(workInProgress);
296+
} else {
297+
// If children might have changed, we have to add them all to the set.
298+
appendAllChildren(newInstance, workInProgress);
262299
}
263300
};
264301
updateHostText = function(
@@ -290,12 +327,9 @@ if (supportsMutation) {
290327
updateHostComponent = function(
291328
current: Fiber,
292329
workInProgress: Fiber,
293-
updatePayload: null | UpdatePayload,
294330
type: Type,
295-
oldProps: Props,
296331
newProps: Props,
297332
rootContainerInstance: Container,
298-
currentHostContext: HostContext,
299333
) {
300334
// Noop
301335
};
@@ -358,39 +392,13 @@ function completeWork(
358392
const rootContainerInstance = getRootHostContainer();
359393
const type = workInProgress.type;
360394
if (current !== null && workInProgress.stateNode != null) {
361-
// If we have an alternate, that means this is an update and we need to
362-
// schedule a side-effect to do the updates.
363-
const oldProps = current.memoizedProps;
364-
if (oldProps !== newProps) {
365-
// If we get updated because one of our children updated, we don't
366-
// have newProps so we'll have to reuse them.
367-
// TODO: Split the update API as separate for the props vs. children.
368-
// Even better would be if children weren't special cased at all tho.
369-
const instance: Instance = workInProgress.stateNode;
370-
const currentHostContext = getHostContext();
371-
// TODO: Experiencing an error where oldProps is null. Suggests a host
372-
// component is hitting the resume path. Figure out why. Possibly
373-
// related to `hidden`.
374-
const updatePayload = prepareUpdate(
375-
instance,
376-
type,
377-
oldProps,
378-
newProps,
379-
rootContainerInstance,
380-
currentHostContext,
381-
);
382-
383-
updateHostComponent(
384-
current,
385-
workInProgress,
386-
updatePayload,
387-
type,
388-
oldProps,
389-
newProps,
390-
rootContainerInstance,
391-
currentHostContext,
392-
);
393-
}
395+
updateHostComponent(
396+
current,
397+
workInProgress,
398+
type,
399+
newProps,
400+
rootContainerInstance,
401+
);
394402

395403
if (current.ref !== workInProgress.ref) {
396404
markRef(workInProgress);

0 commit comments

Comments
 (0)