Do not add additional work to a batch that is already rendering (#13072)
Do not add additional work to a batch that is already rendering (#13072) * Do not add additional work to a batch that is already rendering. Otherwise, the part of the tree that hasn't rendered yet will receive the latest state, but the already rendered part will show the state as it was before the intervening update. * Reduce non-helpfulness of comments
Andrew Clark committed
Jun 19, 2018 at 10:36 UTC
97af3e1f3a6130dc38269b8389da057152868be2
3 files changed
+108
-4
packages/react-reconciler/src/ReactFiberScheduler.js
+22
-1
@@ -1086,6 +1086,10 @@ function renderRoot(root: FiberRoot, isYieldy: boolean): void {
1086
if (__DEV__) {
1087
resetStackAfterFatalErrorInDev();
1088
}
1089
+ // `nextRoot` points to the in-progress root. A non-null value indicates
1090
+ // that we're in the middle of an async render. Set it to null to indicate
1091
+ // there's no more work to be done in the current batch.
1092
+ nextRoot = null;
1093
onFatal(root);
1094
} else if (nextUnitOfWork === null) {
1095
// We reached the root.
@@ -1096,16 +1100,24 @@ function renderRoot(root: FiberRoot, isYieldy: boolean): void {
1100
'caused by a bug in React. Please file an issue.',
1101
);
1102
if ((rootWorkInProgress.effectTag & Incomplete) === NoEffect) {
1103
+ // The root successfully completed.
1104
const didCompleteRoot = true;
1105
stopWorkLoopTimer(interruptedBy, didCompleteRoot);
1106
interruptedBy = null;
1102
- // The root successfully completed.
1107
+ // `nextRoot` points to the in-progress root. A non-null value indicates
1108
+ // that we're in the middle of an async render. Set it to null to indicate
1109
+ // there's no more work to be done in the current batch.
1110
+ nextRoot = null;
1111
onComplete(root, rootWorkInProgress, expirationTime);
1112
} else {
1113
// The root did not complete.
1114
const didCompleteRoot = false;
1115
stopWorkLoopTimer(interruptedBy, didCompleteRoot);
1116
interruptedBy = null;
1117
+ // `nextRoot` points to the in-progress root. A non-null value indicates
1118
+ // that we're in the middle of an async render. Set it to null to indicate
1119
+ // there's no more work to be done in the current batch.
1120
+ nextRoot = null;
1121
markSuspendedPriorityLevel(root, expirationTime, nextRenderDidError);
1122
const suspendedExpirationTime = expirationTime;
1123
const newExpirationTime = root.expirationTime;
@@ -1117,6 +1129,10 @@ function renderRoot(root: FiberRoot, isYieldy: boolean): void {
1129
);
1130
}
1131
} else {
1132
+ // There's still remaining async work in this tree, but we ran out of time
1133
+ // in the current frame. Yield back to the renderer. Unless we're
1134
+ // interrupted by a higher priority update, we'll continue later from where
1135
+ // we left off.
1136
const didCompleteRoot = false;
1137
stopWorkLoopTimer(interruptedBy, didCompleteRoot);
1138
interruptedBy = null;
@@ -1252,6 +1268,11 @@ function computeExpirationForFiber(currentTime: ExpirationTime, fiber: Fiber) {
1268
// This is an async update
1269
expirationTime = computeAsyncExpiration(currentTime);
1270
}
1271
+ // If we're in the middle of rendering a tree, do not update at the same
1272
+ // expiration time that is already rendering.
1273
+ if (nextRoot !== null && expirationTime === nextRenderExpirationTime) {
1274
+ expirationTime += 1;
1275
+ }
1276
} else {
1277
// This is a sync update
1278
expirationTime = Sync;
packages/react-reconciler/src/__tests__/ReactExpiration-test.internal.js
+53
@@ -159,4 +159,57 @@ describe('ReactExpiration', () => {
159
expect(ReactNoop.flush()).toEqual(['B [render]', 'B [commit]']);
160
},
161
);
162
+
163
+ it('cannot update at the same expiration time that is already rendering', () => {
164
+ let store = {text: 'initial'};
165
+ let subscribers = [];
166
+ class Connected extends React.Component {
167
+ state = {text: store.text};
168
+ componentDidMount() {
169
+ subscribers.push(this);
170
+ ReactNoop.yield(`${this.state.text} [${this.props.label}] [commit]`);
171
+ }
172
+ componentDidUpdate() {
173
+ ReactNoop.yield(`${this.state.text} [${this.props.label}] [commit]`);
174
+ }
175
+ render() {
176
+ ReactNoop.yield(`${this.state.text} [${this.props.label}] [render]`);
177
+ return <span prop={this.state.text} />;
178
+ }
179
+ }
180
+
181
+ function App() {
182
+ return (
183
+ <React.Fragment>
184
+ <Connected label="A" />
185
+ <Connected label="B" />
186
+ <Connected label="C" />
187
+ <Connected label="D" />
188
+ </React.Fragment>
189
+ );
190
+ }
191
+
192
+ // Initial mount
193
+ ReactNoop.render(<App />);
194
+ expect(ReactNoop.flush()).toEqual([
195
+ 'initial [A] [render]',
196
+ 'initial [B] [render]',
197
+ 'initial [C] [render]',
198
+ 'initial [D] [render]',
199
+ 'initial [A] [commit]',
200
+ 'initial [B] [commit]',
201
+ 'initial [C] [commit]',
202
+ 'initial [D] [commit]',
203
+ ]);
204
+
205
+ // Partial update
206
+ subscribers.forEach(s => s.setState({text: '1'}));
207
+ ReactNoop.flushThrough(['1 [A] [render]', '1 [B] [render]']);
208
+
209
+ // Before the update can finish, update again. Even though no time has
210
+ // advanced, this update should be given a different expiration time than
211
+ // the currently rendering one. So, C and D should render with 1, not 2.
212
+ subscribers.forEach(s => s.setState({text: '2'}));
213
+ ReactNoop.flushThrough(['1 [C] [render]', '1 [D] [render]']);
214
+ });
215
});
packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js
+33
-3
@@ -163,7 +163,7 @@ describe('ReactIncrementalUpdates', () => {
163
ReactNoop.flushThrough(['a', 'b', 'c']);
164
expect(ReactNoop.getChildren()).toEqual([span('')]);
165
166
- // Schedule some more updates at different priorities{
166
+ // Schedule some more updates at different priorities
167
instance.setState(createUpdate('d'));
168
ReactNoop.flushSync(() => {
169
instance.setState(createUpdate('e'));
@@ -178,7 +178,22 @@ describe('ReactIncrementalUpdates', () => {
178
// they should be processed again, to ensure that the terminal state
179
// is deterministic.
180
ReactNoop.clearYields();
181
- expect(ReactNoop.flush()).toEqual(['a', 'b', 'c', 'd', 'e', 'f', 'g']);
181
+ expect(ReactNoop.flush()).toEqual([
182
+ 'a',
183
+ 'b',
184
+ 'c',
185
+
186
+ // e, f, and g are in a separate batch from a, b, and c because they
187
+ // were scheduled in the middle of a render
188
+ 'e',
189
+ 'f',
190
+ 'g',
191
+
192
+ 'd',
193
+ 'e',
194
+ 'f',
195
+ 'g',
196
+ ]);
197
expect(ReactNoop.getChildren()).toEqual([span('abcdefg')]);
198
});
199
@@ -237,7 +252,22 @@ describe('ReactIncrementalUpdates', () => {
252
// they should be processed again, to ensure that the terminal state
253
// is deterministic.
254
ReactNoop.clearYields();
240
- expect(ReactNoop.flush()).toEqual(['a', 'b', 'c', 'd', 'e', 'f', 'g']);
255
+ expect(ReactNoop.flush()).toEqual([
256
+ 'a',
257
+ 'b',
258
+ 'c',
259
+
260
+ // e, f, and g are in a separate batch from a, b, and c because they
261
+ // were scheduled in the middle of a render
262
+ 'e',
263
+ 'f',
264
+ 'g',
265
+
266
+ 'd',
267
+ 'e',
268
+ 'f',
269
+ 'g',
270
+ ]);
271
expect(ReactNoop.getChildren()).toEqual([span('fg')]);
272
});
273