Task work inside batched updates is always sync, even for initial mounts
Behavior now matches Stack. It's unfortunate that this prevents us from unifying SynchronousPriority and TaskPriority.
Andrew Clark committed
Jun 15, 2017 at 19:28 UTC
dcc02dd0f12fb4b9b4b910aef6c5808dc761dc22
5 files changed
+135
-13
scripts/fiber/tests-passing.txt
+2
@@ -1309,6 +1309,7 @@ src/renderers/dom/shared/__tests__/ReactMount-test.js
1309
* should warn if render removes React-rendered children
1310
* should warn if the unmounted node was rendered by another copy of React
1311
* passes the correct callback context
1312
+* initial mount is sync inside batchedUpdates, but task work is deferred until the end of the batch
1313
1314
src/renderers/dom/shared/__tests__/ReactMountDestruction-test.js
1315
* should destroy a react root upon request
@@ -1832,6 +1833,7 @@ src/renderers/shared/fiber/__tests__/ReactIncrementalPerf-test.js
1833
* deduplicates lifecycle names during commit to reduce overhead
1834
* supports coroutines
1835
* supports portals
1836
+* does not schedule an extra callback if setState is called during a synchronous commit phase
1837
1838
src/renderers/shared/fiber/__tests__/ReactIncrementalReflection-test.js
1839
* handles isMounted even when the initial render is deferred
src/renderers/dom/shared/__tests__/ReactMount-test.js
+35
@@ -310,4 +310,39 @@ describe('ReactMount', () => {
310
311
expect(calls).toBe(5);
312
});
313
+
314
+ it('initial mount is sync inside batchedUpdates, but task work is deferred until the end of the batch', () => {
315
+ var container1 = document.createElement('div');
316
+ var container2 = document.createElement('div');
317
+
318
+ class Foo extends React.Component {
319
+ state = {active: false};
320
+ componentDidMount() {
321
+ this.setState({active: true});
322
+ }
323
+ render() {
324
+ return (
325
+ <div>{this.props.children + (this.state.active ? '!' : '')}</div>
326
+ );
327
+ }
328
+ }
329
+
330
+ ReactDOM.render(<div>1</div>, container1);
331
+
332
+ ReactDOM.unstable_batchedUpdates(() => {
333
+ // Update. Does not flush yet.
334
+ ReactDOM.render(<div>2</div>, container1);
335
+ expect(container1.textContent).toEqual('1');
336
+
337
+ // Initial mount on another root. Should flush immediately.
338
+ ReactDOM.render(<Foo>a</Foo>, container2);
339
+ // The update did not flush yet.
340
+ expect(container1.textContent).toEqual('1');
341
+ // The initial mount flushed, but not the update scheduled in cDU.
342
+ expect(container2.textContent).toEqual('a');
343
+ });
344
+ // All updates have flushed.
345
+ expect(container1.textContent).toEqual('2');
346
+ expect(container2.textContent).toEqual('a!');
347
+ });
348
});
src/renderers/shared/fiber/ReactFiberScheduler.js
+68
-13
@@ -195,6 +195,10 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
195
// Keeps track of whether we should should batch sync updates.
196
let isBatchingUpdates: boolean = false;
197
198
+ // This is needed for the weird case where the initial mount is synchronous
199
+ // even inside batchedUpdates :(
200
+ let isUnbatchingUpdates: boolean = false;
201
+
202
// The next work in progress fiber that we're currently working on.
203
let nextUnitOfWork: Fiber | null = null;
204
let nextPriorityLevel: PriorityLevel = NoWork;
@@ -759,7 +763,10 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
763
}
764
}
765
762
- function workLoop(deadline: Deadline | null) {
766
+ function workLoop(
767
+ minPriorityLevel: PriorityLevel,
768
+ deadline: Deadline | null,
769
+ ) {
770
// Clear any errors.
771
clearErrors();
772
@@ -771,6 +778,7 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
778
while (
779
nextUnitOfWork !== null &&
780
nextPriorityLevel !== NoWork &&
781
+ nextPriorityLevel <= minPriorityLevel &&
782
nextPriorityLevel <= TaskPriority
783
) {
784
nextUnitOfWork = performUnitOfWork(nextUnitOfWork);
@@ -788,6 +796,7 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
796
nextUnitOfWork !== null &&
797
!deadlineHasExpired &&
798
nextPriorityLevel !== NoWork &&
799
+ nextPriorityLevel <= minPriorityLevel &&
800
nextPriorityLevel >= HighPriority
801
) {
802
if (deadline.timeRemaining() > timeHeuristicForUnitOfWork) {
@@ -815,7 +824,14 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
824
}
825
}
826
818
- function performWork(deadline: Deadline | null) {
827
+ function performDeferredWork(deadline: Deadline) {
828
+ performWork(OffscreenPriority, deadline);
829
+ }
830
+
831
+ function performWork(
832
+ minPriorityLevel: PriorityLevel,
833
+ deadline: Deadline | null,
834
+ ) {
835
if (__DEV__) {
836
startWorkLoopTimer();
837
}
@@ -849,10 +865,16 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
865
priorityContextBeforeReconciliation = priorityContext;
866
let error = null;
867
if (__DEV__) {
852
- error = invokeGuardedCallback(null, workLoop, null, deadline);
868
+ error = invokeGuardedCallback(
869
+ null,
870
+ workLoop,
871
+ null,
872
+ minPriorityLevel,
873
+ deadline,
874
+ );
875
} else {
876
try {
855
- workLoop(deadline);
877
+ workLoop(minPriorityLevel, deadline);
878
} catch (e) {
879
error = e;
880
}
@@ -903,7 +925,10 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
925
case TaskPriority:
926
// We have remaining synchronous or task work. Keep performing it,
927
// regardless of whether we're inside a callback.
906
- continue;
928
+ if (nextPriorityLevel <= minPriorityLevel) {
929
+ continue;
930
+ }
931
+ break;
932
case HighPriority:
933
case LowPriority:
934
case OffscreenPriority:
@@ -914,7 +939,10 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
939
hasRemainingAsyncWork = true;
940
} else {
941
// We are inside a callback.
917
- if (!deadlineHasExpired) {
942
+ if (
943
+ !deadlineHasExpired &&
944
+ nextPriorityLevel <= minPriorityLevel
945
+ ) {
946
// We still have time. Keep working.
947
continue;
948
}
@@ -939,7 +967,7 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
967
}
968
// If there's remaining async work, make sure we schedule another callback.
969
if (hasRemainingAsyncWork && !isCallbackScheduled) {
942
- scheduleDeferredCallback(performWork);
970
+ scheduleDeferredCallback(performDeferredWork);
971
isCallbackScheduled = true;
972
}
973
@@ -1264,11 +1292,34 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
1292
if (node.tag === HostRoot) {
1293
const root: FiberRoot = (node.stateNode: any);
1294
scheduleRoot(root, priorityLevel);
1267
- if (priorityLevel === SynchronousPriority) {
1268
- performWork(null);
1269
- } else if (priorityLevel !== NoWork && !isCallbackScheduled) {
1270
- scheduleDeferredCallback(performWork);
1271
- isCallbackScheduled = true;
1295
+ if (!isPerformingWork) {
1296
+ switch (priorityLevel) {
1297
+ case SynchronousPriority:
1298
+ // Perform this update now.
1299
+ if (isUnbatchingUpdates) {
1300
+ // We're inside unbatchedUpdates, which is inside either
1301
+ // batchedUpdates or a lifecycle. We should only flush
1302
+ // synchronous work, not task work.
1303
+ performWork(SynchronousPriority, null);
1304
+ } else {
1305
+ // Flush both synchronous and task work.
1306
+ performWork(TaskPriority, null);
1307
+ }
1308
+ break;
1309
+ case TaskPriority:
1310
+ invariant(
1311
+ isBatchingUpdates,
1312
+ 'Task updates can only be scheduled as a nested update or ' +
1313
+ 'inside batchedUpdates.',
1314
+ );
1315
+ break;
1316
+ default:
1317
+ // Schedule a callback to perform the work later.
1318
+ if (!isCallbackScheduled) {
1319
+ scheduleDeferredCallback(performDeferredWork);
1320
+ isCallbackScheduled = true;
1321
+ }
1322
+ }
1323
}
1324
} else {
1325
if (__DEV__) {
@@ -1335,18 +1386,22 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
1386
// If we're not already inside a batch, we need to flush any task work
1387
// that was created by the user-provided function.
1388
if (!isPerformingWork && !isBatchingUpdates) {
1338
- performWork(null);
1389
+ performWork(TaskPriority, null);
1390
}
1391
}
1392
}
1393
1394
function unbatchedUpdates<A>(fn: () => A): A {
1395
+ const previousIsUnbatchingUpdates = isUnbatchingUpdates;
1396
const previousIsBatchingUpdates = isBatchingUpdates;
1397
+ // This is only true if we're nested inside batchedUpdates.
1398
+ isUnbatchingUpdates = isBatchingUpdates;
1399
isBatchingUpdates = false;
1400
try {
1401
return fn();
1402
} finally {
1403
isBatchingUpdates = previousIsBatchingUpdates;
1404
+ isUnbatchingUpdates = previousIsUnbatchingUpdates;
1405
}
1406
}
1407
src/renderers/shared/fiber/__tests__/ReactIncrementalPerf-test.js
+16
@@ -487,4 +487,20 @@ describe('ReactDebugFiberPerf', () => {
487
ReactNoop.flush();
488
expect(getFlameChart()).toMatchSnapshot();
489
});
490
+
491
+ it('does not schedule an extra callback if setState is called during a synchronous commit phase', () => {
492
+ class Component extends React.Component {
493
+ state = {step: 1};
494
+ componentDidMount() {
495
+ this.setState({step: 2});
496
+ }
497
+ render() {
498
+ return <span prop={this.state.step} />;
499
+ }
500
+ }
501
+ ReactNoop.syncUpdates(() => {
502
+ ReactNoop.render(<Component />);
503
+ });
504
+ expect(getFlameChart()).toMatchSnapshot();
505
+ });
506
});
src/renderers/shared/fiber/__tests__/__snapshots__/ReactIncrementalPerf-test.js.snap
+14
@@ -67,6 +67,20 @@ exports[`ReactDebugFiberPerf deduplicates lifecycle names during commit to reduc
67
"
68
`;
69
70
+exports[`ReactDebugFiberPerf does not schedule an extra callback if setState is called during a synchronous commit phase 1`] = `
71
+"⛔ (React Tree Reconciliation) Warning: There were cascading updates
72
+ ⚛ Component [mount]
73
+ ⛔ (Committing Changes) Warning: Lifecycle hook scheduled a cascading update
74
+ ⚛ (Committing Host Effects: 1 Total)
75
+ ⚛ (Calling Lifecycle Methods: 1 Total)
76
+ ⛔ Component.componentDidMount Warning: Scheduled a cascading update
77
+ ⚛ Component [update]
78
+ ⛔ (Committing Changes) Warning: Caused by a cascading update in earlier commit
79
+ ⚛ (Committing Host Effects: 1 Total)
80
+ ⚛ (Calling Lifecycle Methods: 1 Total)
81
+"
82
+`;
83
+
84
exports[`ReactDebugFiberPerf does not treat setState from cWM or cWRP as cascading 1`] = `
85
"// Should not print a warning
86
⚛ (React Tree Reconciliation)