Sync setStates inside willUpdate and didUpdate should flush at same time (#11212)
In sync mode, we downgrade sync priority work to task work when we're in the commit phase, but not in the render phase. That means if you schedule updates in both phases, the render phase update will flush first, and the commit phase update will flush after that. What should really happen is that both updates flush at the same time. To solve this, updates in the commit phase are now given sync priority. The distinction between task and sync really only exists to account for a historical quirk in the behavior of top-level mounts. (Refer to the test case titled "initial mount is sync inside batchedUpdates".) Ideally, there would only be one priority for both sync and task. This gets us closer to that model, while still accounting for top-level mounts.
Andrew Clark committed
Oct 12, 2017 at 21:05 UTC
339d6cb32bc80dd8ed9d500c634a47fa0dafc8d1
2 files changed
+107
-2
src/renderers/__tests__/ReactUpdates-test.js
+102
@@ -1072,6 +1072,108 @@ describe('ReactUpdates', () => {
1072
expect(ops).toEqual(['Hello', '']);
1073
});
1074
1075
+ it(
1076
+ 'in sync mode, updates in componentWillUpdate and componentDidUpdate ' +
1077
+ 'should both flush in the immediately subsequent commit',
1078
+ () => {
1079
+ let ops = [];
1080
+ class Foo extends React.Component {
1081
+ state = {a: false, b: false};
1082
+ componentWillUpdate(_, nextState) {
1083
+ if (!nextState.a) {
1084
+ this.setState({a: true});
1085
+ }
1086
+ }
1087
+ componentDidUpdate() {
1088
+ ops.push('Foo updated');
1089
+ if (!this.state.b) {
1090
+ this.setState({b: true});
1091
+ }
1092
+ }
1093
+ render() {
1094
+ ops.push(`a: ${this.state.a}, b: ${this.state.b}`);
1095
+ return null;
1096
+ }
1097
+ }
1098
+
1099
+ const container = document.createElement('div');
1100
+ // Mount
1101
+ ReactDOM.render(<Foo />, container);
1102
+ // Root update
1103
+ ReactDOM.render(<Foo />, container);
1104
+ expect(ops).toEqual([
1105
+ // Mount
1106
+ 'a: false, b: false',
1107
+ // Root update
1108
+ 'a: false, b: false',
1109
+ 'Foo updated',
1110
+ // Subsequent update (both a and b should have flushed)
1111
+ 'a: true, b: true',
1112
+ 'Foo updated',
1113
+ // There should not be any additional updates
1114
+ ]);
1115
+ },
1116
+ );
1117
+
1118
+ it(
1119
+ 'in sync mode, updates in componentWillUpdate and componentDidUpdate ' +
1120
+ '(on a sibling) should both flush in the immediately subsequent commit',
1121
+ () => {
1122
+ let ops = [];
1123
+ class Foo extends React.Component {
1124
+ state = {a: false};
1125
+ componentWillUpdate(_, nextState) {
1126
+ if (!nextState.a) {
1127
+ this.setState({a: true});
1128
+ }
1129
+ }
1130
+ componentDidUpdate() {
1131
+ ops.push('Foo updated');
1132
+ }
1133
+ render() {
1134
+ ops.push(`a: ${this.state.a}`);
1135
+ return null;
1136
+ }
1137
+ }
1138
+
1139
+ class Bar extends React.Component {
1140
+ state = {b: false};
1141
+ componentDidUpdate() {
1142
+ ops.push('Bar updated');
1143
+ if (!this.state.b) {
1144
+ this.setState({b: true});
1145
+ }
1146
+ }
1147
+ render() {
1148
+ ops.push(`b: ${this.state.b}`);
1149
+ return null;
1150
+ }
1151
+ }
1152
+
1153
+ const container = document.createElement('div');
1154
+ // Mount
1155
+ ReactDOM.render(<div><Foo /><Bar /></div>, container);
1156
+ // Root update
1157
+ ReactDOM.render(<div><Foo /><Bar /></div>, container);
1158
+ expect(ops).toEqual([
1159
+ // Mount
1160
+ 'a: false',
1161
+ 'b: false',
1162
+ // Root update
1163
+ 'a: false',
1164
+ 'b: false',
1165
+ 'Foo updated',
1166
+ 'Bar updated',
1167
+ // Subsequent update (both a and b should have flushed)
1168
+ 'a: true',
1169
+ 'b: true',
1170
+ 'Foo updated',
1171
+ 'Bar updated',
1172
+ // There should not be any additional updates
1173
+ ]);
1174
+ },
1175
+ );
1176
+
1177
it('does not re-render if state update is null', () => {
1178
let container = document.createElement('div');
1179
src/renderers/shared/fiber/ReactFiberScheduler.js
+5
-2
@@ -1494,7 +1494,7 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
1494
if (isCommitting) {
1495
// Updates that occur during the commit phase should have task priority
1496
// by default.
1497
- expirationTime = Task;
1497
+ expirationTime = Sync;
1498
} else {
1499
// Updates during the render phase should expire at the same time as
1500
// the work that is being rendered.
@@ -1517,7 +1517,10 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
1517
}
1518
}
1519
1520
- if (expirationTime === Sync && (isCommitting || isBatchingUpdates)) {
1520
+ if (
1521
+ expirationTime === Sync &&
1522
+ (isBatchingUpdates || (isUnbatchingUpdates && isCommitting))
1523
+ ) {
1524
// If we're in a batch, or in the commit phase, downgrade sync to task
1525
return Task;
1526
}