Better fix for base state bug (#11273)
Andrew Clark committed
Oct 18, 2017 at 15:11 UTC
d9647282e6f3625c0e5cf19c89b199a2706bce01
2 files changed
+41
-11
src/renderers/__tests__/ReactUpdates-test.js
+27
@@ -1174,6 +1174,33 @@ describe('ReactUpdates', () => {
1174
},
1175
);
1176
1177
+ it('uses correct base state for setState inside render phase', () => {
1178
+ spyOn(console, 'error');
1179
+
1180
+ let ops = [];
1181
+
1182
+ class Foo extends React.Component {
1183
+ state = {step: 0};
1184
+ render() {
1185
+ const memoizedStep = this.state.step;
1186
+ this.setState(baseState => {
1187
+ const baseStep = baseState.step;
1188
+ ops.push(`base: ${baseStep}, memoized: ${memoizedStep}`);
1189
+ return baseStep === 0 ? {step: 1} : null;
1190
+ });
1191
+ return null;
1192
+ }
1193
+ }
1194
+
1195
+ const container = document.createElement('div');
1196
+ ReactDOM.render(<Foo />, container);
1197
+ expect(ops).toEqual(['base: 0, memoized: 0', 'base: 1, memoized: 1']);
1198
+ expect(console.error.calls.count()).toBe(1);
1199
+ expect(console.error.calls.argsFor(0)[0]).toContain(
1200
+ 'Cannot update during an existing state transition',
1201
+ );
1202
+ });
1203
+
1204
it('does not re-render if state update is null', () => {
1205
let container = document.createElement('div');
1206
src/renderers/shared/fiber/ReactFiberUpdateQueue.js
+14
-11
@@ -113,16 +113,18 @@ function insertUpdateIntoFiber<State>(
113
const alternateFiber = fiber.alternate;
114
let queue1 = fiber.updateQueue;
115
if (queue1 === null) {
116
- queue1 = fiber.updateQueue = createUpdateQueue(fiber.memoizedState);
116
+ // TODO: We don't know what the base state will be until we begin work.
117
+ // It depends on which fiber is the next current. Initialize with an empty
118
+ // base state, then set to the memoizedState when rendering. Not super
119
+ // happy with this approach.
120
+ queue1 = fiber.updateQueue = createUpdateQueue(null);
121
}
122
123
let queue2;
124
if (alternateFiber !== null) {
125
queue2 = alternateFiber.updateQueue;
126
if (queue2 === null) {
123
- queue2 = alternateFiber.updateQueue = createUpdateQueue(
124
- alternateFiber.memoizedState,
125
- );
127
+ queue2 = alternateFiber.updateQueue = createUpdateQueue(null);
128
}
129
} else {
130
queue2 = null;
@@ -198,12 +200,7 @@ function processUpdateQueue<State>(
200
// We need to create a work-in-progress queue, by cloning the current queue.
201
const currentQueue = queue;
202
queue = workInProgress.updateQueue = {
201
- // If we hit this clone path, the work-in-progress update queue is
202
- // conceptually empty. Which means its base state is the same as its
203
- // memoized state. This usually the same as the current queue's base
204
- // state, but could be different if setState was called during a child's
205
- // render phase.
206
- baseState: workInProgress.memoizedState,
203
+ baseState: currentQueue.baseState,
204
expirationTime: currentQueue.expirationTime,
205
first: currentQueue.first,
206
last: currentQueue.last,
@@ -224,7 +221,13 @@ function processUpdateQueue<State>(
221
// increase this accordingly.
222
queue.expirationTime = NoWork;
223
227
- let state = queue.baseState;
224
+ // TODO: We don't know what the base state will be until we begin work.
225
+ // It depends on which fiber is the next current. Initialize with an empty
226
+ // base state, then set to the memoizedState when rendering. Not super
227
+ // happy with this approach.
228
+ let state = queue.baseState === null
229
+ ? workInProgress.memoizedState
230
+ : queue.baseState;
231
let dontMutatePrevState = true;
232
let update = queue.first;
233
let didSkip = false;