Fix host bailout for the persistent mode (#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
Dan Abramov committed
Sep 10, 2018 at 19:05 UTC
f260b14a8f1f95a75f5595c9da392cc640e4ad88
5 files changed
+353
-83
packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js
+39
@@ -237,6 +237,45 @@ describe('ReactFabric', () => {
237
expect(FabricUIManager.__dumpHierarchyForJestTestsOnly()).toMatchSnapshot();
238
});
239
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
+
279
it('calls setState with no arguments', () => {
280
let mockArgs;
281
class Component extends React.Component {
packages/react-native-renderer/src/__tests__/__snapshots__/ReactFabric-test.internal.js.snap
+52
@@ -1,5 +1,57 @@
1
// Jest Snapshot v1, https://goo.gl/fbAQLP
2
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
+
55
exports[`ReactFabric renders and reorders children 1`] = `
56
"11
57
RCTView null
packages/react-noop-renderer/src/createReactNoop.js
+22
-8
@@ -46,6 +46,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
46
let instanceCounter = 0;
47
let hostDiffCounter = 0;
48
let hostUpdateCounter = 0;
49
+ let hostCloneCounter = 0;
50
51
function appendChildToContainerOrInstance(
52
parentInstance: Container | Instance,
@@ -370,6 +371,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
371
value: clone.id,
372
enumerable: false,
373
});
374
+ hostCloneCounter++;
375
return clone;
376
},
377
@@ -579,21 +581,33 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
581
582
flushWithHostCounters(
583
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
+ |} {
593
hostDiffCounter = 0;
594
hostUpdateCounter = 0;
595
+ hostCloneCounter = 0;
596
try {
597
ReactNoop.flush();
590
- return {
591
- hostDiffCounter,
592
- hostUpdateCounter,
593
- };
598
+ return useMutation
599
+ ? {
600
+ hostDiffCounter,
601
+ hostUpdateCounter,
602
+ }
603
+ : {
604
+ hostDiffCounter,
605
+ hostCloneCounter,
606
+ };
607
} finally {
608
hostDiffCounter = 0;
609
hostUpdateCounter = 0;
610
+ hostCloneCounter = 0;
611
}
612
},
613
packages/react-reconciler/src/ReactFiberCompleteWork.js
+83
-75
@@ -14,10 +14,8 @@ import type {
14
Instance,
15
Type,
16
Props,
17
- UpdatePayload,
17
Container,
18
ChildSet,
20
- HostContext,
19
} from './ReactFiberHostConfig';
20
21
import {
@@ -126,13 +124,36 @@ if (supportsMutation) {
124
updateHostComponent = function(
125
current: Fiber,
126
workInProgress: Fiber,
129
- updatePayload: null | UpdatePayload,
127
type: Type,
131
- oldProps: Props,
128
newProps: Props,
129
rootContainerInstance: Container,
134
- currentHostContext: HostContext,
130
) {
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
+ );
157
// TODO: Type this specific to this type of component.
158
workInProgress.updateQueue = (updatePayload: any);
159
// If the update payload indicates that there is a change or if there
@@ -211,54 +232,70 @@ if (supportsMutation) {
232
updateHostComponent = function(
233
current: Fiber,
234
workInProgress: Fiber,
214
- updatePayload: null | UpdatePayload,
235
type: Type,
216
- oldProps: Props,
236
newProps: Props,
237
rootContainerInstance: Container,
219
- currentHostContext: HostContext,
238
) {
239
+ const currentInstance = current.stateNode;
240
+ const oldProps = current.memoizedProps;
241
// If there are no effects associated with this node, then none of our children had any updates.
242
// This guarantees that we can reuse all of them.
243
const childrenUnchanged = workInProgress.firstEffect === null;
224
- const currentInstance = current.stateNode;
225
- if (childrenUnchanged && updatePayload === null) {
244
+ if (childrenUnchanged && oldProps === newProps) {
245
// No changes, just reuse the existing instance.
246
// Note that this might release a previous clone.
247
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,
256
type,
257
oldProps,
258
newProps,
237
- workInProgress,
238
- childrenUnchanged,
239
- recyclableInstance,
259
+ rootContainerInstance,
260
+ currentHostContext,
261
);
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);
299
}
300
};
301
updateHostText = function(
@@ -290,12 +327,9 @@ if (supportsMutation) {
327
updateHostComponent = function(
328
current: Fiber,
329
workInProgress: Fiber,
293
- updatePayload: null | UpdatePayload,
330
type: Type,
295
- oldProps: Props,
331
newProps: Props,
332
rootContainerInstance: Container,
298
- currentHostContext: HostContext,
333
) {
334
// Noop
335
};
@@ -358,39 +392,13 @@ function completeWork(
392
const rootContainerInstance = getRootHostContainer();
393
const type = workInProgress.type;
394
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
+ );
402
403
if (current.ref !== workInProgress.ref) {
404
markRef(workInProgress);
packages/react-reconciler/src/__tests__/ReactPersistentUpdatesMinimalism-test.js
new
+157
@@ -0,0 +1,157 @@
1
+/**
2
+ * Copyright (c) Facebook, Inc. and its affiliates.
3
+ *
4
+ * This source code is licensed under the MIT license found in the
5
+ * LICENSE file in the root directory of this source tree.
6
+ *
7
+ * @emails react-core
8
+ * @jest-environment node
9
+ */
10
+
11
+'use strict';
12
+
13
+let React;
14
+let ReactNoopPersistent;
15
+
16
+describe('ReactPersistentUpdatesMinimalism', () => {
17
+ beforeEach(() => {
18
+ jest.resetModules();
19
+ React = require('react');
20
+ ReactNoopPersistent = require('react-noop-renderer/persistent');
21
+ });
22
+
23
+ it('should render a simple component', () => {
24
+ function Child() {
25
+ return <div>Hello World</div>;
26
+ }
27
+
28
+ function Parent() {
29
+ return <Child />;
30
+ }
31
+
32
+ ReactNoopPersistent.render(<Parent />);
33
+ expect(ReactNoopPersistent.flushWithHostCounters()).toEqual({
34
+ hostDiffCounter: 0,
35
+ hostCloneCounter: 0,
36
+ });
37
+
38
+ ReactNoopPersistent.render(<Parent />);
39
+ expect(ReactNoopPersistent.flushWithHostCounters()).toEqual({
40
+ hostDiffCounter: 1,
41
+ hostCloneCounter: 1,
42
+ });
43
+ });
44
+
45
+ it('should not diff referentially equal host elements', () => {
46
+ function Leaf(props) {
47
+ return (
48
+ <span>
49
+ hello
50
+ <b />
51
+ {props.name}
52
+ </span>
53
+ );
54
+ }
55
+
56
+ const constEl = (
57
+ <div>
58
+ <Leaf name="world" />
59
+ </div>
60
+ );
61
+
62
+ function Child() {
63
+ return constEl;
64
+ }
65
+
66
+ function Parent() {
67
+ return <Child />;
68
+ }
69
+
70
+ ReactNoopPersistent.render(<Parent />);
71
+ expect(ReactNoopPersistent.flushWithHostCounters()).toEqual({
72
+ hostDiffCounter: 0,
73
+ hostCloneCounter: 0,
74
+ });
75
+
76
+ ReactNoopPersistent.render(<Parent />);
77
+ expect(ReactNoopPersistent.flushWithHostCounters()).toEqual({
78
+ hostDiffCounter: 0,
79
+ hostCloneCounter: 0,
80
+ });
81
+ });
82
+
83
+ it('should not diff parents of setState targets', () => {
84
+ let childInst;
85
+
86
+ function Leaf(props) {
87
+ return (
88
+ <span>
89
+ hello
90
+ <b />
91
+ {props.name}
92
+ </span>
93
+ );
94
+ }
95
+
96
+ class Child extends React.Component {
97
+ state = {name: 'Batman'};
98
+ render() {
99
+ childInst = this;
100
+ return (
101
+ <div>
102
+ <Leaf name={this.state.name} />
103
+ </div>
104
+ );
105
+ }
106
+ }
107
+
108
+ function Parent() {
109
+ return (
110
+ <section>
111
+ <div>
112
+ <Leaf name="world" />
113
+ <Child />
114
+ <hr />
115
+ <Leaf name="world" />
116
+ </div>
117
+ </section>
118
+ );
119
+ }
120
+
121
+ ReactNoopPersistent.render(<Parent />);
122
+ expect(ReactNoopPersistent.flushWithHostCounters()).toEqual({
123
+ hostDiffCounter: 0,
124
+ hostCloneCounter: 0,
125
+ });
126
+
127
+ childInst.setState({name: 'Robin'});
128
+ expect(ReactNoopPersistent.flushWithHostCounters()).toEqual({
129
+ // section > div > Child > div
130
+ // section > div > Child > Leaf > span
131
+ // section > div > Child > Leaf > span > b
132
+ hostDiffCounter: 3,
133
+ // section
134
+ // section > div
135
+ // section > div > Child > div
136
+ // section > div > Child > Leaf > span
137
+ // section > div > Child > Leaf > span > b
138
+ hostCloneCounter: 5,
139
+ });
140
+
141
+ ReactNoopPersistent.render(<Parent />);
142
+ expect(ReactNoopPersistent.flushWithHostCounters()).toEqual({
143
+ // Parent > section
144
+ // Parent > section > div
145
+ // Parent > section > div > Leaf > span
146
+ // Parent > section > div > Leaf > span > b
147
+ // Parent > section > div > Child > div
148
+ // Parent > section > div > Child > div > Leaf > span
149
+ // Parent > section > div > Child > div > Leaf > span > b
150
+ // Parent > section > div > hr
151
+ // Parent > section > div > Leaf > span
152
+ // Parent > section > div > Leaf > span > b
153
+ hostDiffCounter: 10,
154
+ hostCloneCounter: 10,
155
+ });
156
+ });
157
+});