Use global state for `hasForceUpdate` instead of persisting to queue (#12808)
* Use global state for `hasForceUpdate` instead of persisting to queue Fixes a bug where `hasForceUpdate` was not reset on commit. Ideally we'd use a tuple and return `hasForceUpdate` from `processUpdateQueue`. * Remove underscore and add comment * Remove temporary variables
Andrew Clark committed
May 14, 2018 at 19:18 UTC
73f59e6f3152f0579625ecd50caa936920c8e5eb
3 files changed
+67
-39
packages/react-reconciler/src/ReactFiberClassComponent.js
+28
-32
@@ -32,6 +32,8 @@ import {StrictMode} from './ReactTypeOfMode';
32
import {
33
enqueueUpdate,
34
processUpdateQueue,
35
+ checkHasForceUpdateAfterProcessing,
36
+ resetHasForceUpdateBeforeProcessing,
37
createUpdate,
38
ReplaceState,
39
ForceUpdate,
@@ -235,14 +237,6 @@ export default function(
237
newState,
238
newContext,
239
) {
238
- if (
239
- workInProgress.updateQueue !== null &&
240
- workInProgress.updateQueue.hasForceUpdate
241
- ) {
242
- // If forceUpdate was called, disregard sCU.
243
- return true;
244
- }
245
-
240
const instance = workInProgress.stateNode;
241
const ctor = workInProgress.type;
242
if (typeof instance.shouldComponentUpdate === 'function') {
@@ -789,6 +783,8 @@ export default function(
783
}
784
}
785
786
+ resetHasForceUpdateBeforeProcessing();
787
+
788
const oldState = workInProgress.memoizedState;
789
let newState = (instance.state = oldState);
790
let updateQueue = workInProgress.updateQueue;
@@ -806,10 +802,7 @@ export default function(
802
oldProps === newProps &&
803
oldState === newState &&
804
!hasContextChanged() &&
809
- !(
810
- workInProgress.updateQueue !== null &&
811
- workInProgress.updateQueue.hasForceUpdate
812
- )
805
+ !checkHasForceUpdateAfterProcessing()
806
) {
807
// If an update was already in progress, we should schedule an Update
808
// effect even though we're bailing out, so that cWU/cDU are called.
@@ -828,14 +821,16 @@ export default function(
821
newState = workInProgress.memoizedState;
822
}
823
831
- const shouldUpdate = checkShouldComponentUpdate(
832
- workInProgress,
833
- oldProps,
834
- newProps,
835
- oldState,
836
- newState,
837
- newContext,
838
- );
824
+ const shouldUpdate =
825
+ checkHasForceUpdateAfterProcessing() ||
826
+ checkShouldComponentUpdate(
827
+ workInProgress,
828
+ oldProps,
829
+ newProps,
830
+ oldState,
831
+ newState,
832
+ newContext,
833
+ );
834
835
if (shouldUpdate) {
836
// In order to support react-lifecycles-compat polyfilled components,
@@ -922,6 +917,8 @@ export default function(
917
}
918
}
919
920
+ resetHasForceUpdateBeforeProcessing();
921
+
922
const oldState = workInProgress.memoizedState;
923
let newState = (instance.state = oldState);
924
let updateQueue = workInProgress.updateQueue;
@@ -940,10 +937,7 @@ export default function(
937
oldProps === newProps &&
938
oldState === newState &&
939
!hasContextChanged() &&
943
- !(
944
- workInProgress.updateQueue !== null &&
945
- workInProgress.updateQueue.hasForceUpdate
946
- )
940
+ !checkHasForceUpdateAfterProcessing()
941
) {
942
// If an update was already in progress, we should schedule an Update
943
// effect even though we're bailing out, so that cWU/cDU are called.
@@ -977,14 +971,16 @@ export default function(
971
}
972
}
973
980
- const shouldUpdate = checkShouldComponentUpdate(
981
- workInProgress,
982
- oldProps,
983
- newProps,
984
- oldState,
985
- newState,
986
- newContext,
987
- );
974
+ const shouldUpdate =
975
+ checkHasForceUpdateAfterProcessing() ||
976
+ checkShouldComponentUpdate(
977
+ workInProgress,
978
+ oldProps,
979
+ newProps,
980
+ oldState,
981
+ newState,
982
+ newContext,
983
+ );
984
985
if (shouldUpdate) {
986
// In order to support react-lifecycles-compat polyfilled components,
packages/react-reconciler/src/ReactUpdateQueue.js
+16
-7
@@ -131,9 +131,6 @@ export type UpdateQueue<State> = {
131
132
firstCapturedEffect: Update<State> | null,
133
lastCapturedEffect: Update<State> | null,
134
-
135
- // TODO: Workaround for lack of tuples. Could use global state instead.
136
- hasForceUpdate: boolean,
134
};
135
136
export const UpdateState = 0;
@@ -141,6 +138,11 @@ export const ReplaceState = 1;
138
export const ForceUpdate = 2;
139
export const CaptureUpdate = 3;
140
141
+// Global state that is reset at the beginning of calling `processUpdateQueue`.
142
+// It should only be read right after calling `processUpdateQueue`, via
143
+// `checkHasForceUpdateAfterProcessing`.
144
+let hasForceUpdate = false;
145
+
146
let didWarnUpdateInsideUpdate;
147
let currentlyProcessingQueue;
148
export let resetCurrentlyProcessingQueue;
@@ -164,7 +166,6 @@ export function createUpdateQueue<State>(baseState: State): UpdateQueue<State> {
166
lastEffect: null,
167
firstCapturedEffect: null,
168
lastCapturedEffect: null,
167
- hasForceUpdate: false,
169
};
170
return queue;
171
}
@@ -183,8 +184,6 @@ function cloneUpdateQueue<State>(
184
firstCapturedUpdate: null,
185
lastCapturedUpdate: null,
186
186
- hasForceUpdate: false,
187
-
187
firstEffect: null,
188
lastEffect: null,
189
@@ -423,7 +422,7 @@ function getStateFromUpdate<State>(
422
return Object.assign({}, prevState, partialState);
423
}
424
case ForceUpdate: {
426
- queue.hasForceUpdate = true;
425
+ hasForceUpdate = true;
426
return prevState;
427
}
428
}
@@ -437,6 +436,8 @@ export function processUpdateQueue<State>(
436
instance: any,
437
renderExpirationTime: ExpirationTime,
438
): void {
439
+ hasForceUpdate = false;
440
+
441
if (
442
queue.expirationTime === NoWork ||
443
queue.expirationTime > renderExpirationTime
@@ -595,6 +596,14 @@ function callCallback(callback, context) {
596
callback.call(context);
597
}
598
599
+export function resetHasForceUpdateBeforeProcessing() {
600
+ hasForceUpdate = false;
601
+}
602
+
603
+export function checkHasForceUpdateAfterProcessing(): boolean {
604
+ return hasForceUpdate;
605
+}
606
+
607
export function commitUpdateQueue<State>(
608
finishedWork: Fiber,
609
finishedQueue: UpdateQueue<State>,
packages/react-reconciler/src/__tests__/ReactIncremental-test.internal.js
+23
@@ -1115,6 +1115,29 @@ describe('ReactIncremental', () => {
1115
expect(ops).toEqual(['Foo', 'Bar', 'Baz', 'Bar', 'Baz']);
1116
});
1117
1118
+ it('should clear forceUpdate after update is flushed', () => {
1119
+ let a = 0;
1120
+
1121
+ class Foo extends React.PureComponent {
1122
+ render() {
1123
+ const msg = `A: ${a}, B: ${this.props.b}`;
1124
+ ReactNoop.yield(msg);
1125
+ return msg;
1126
+ }
1127
+ }
1128
+
1129
+ const foo = React.createRef(null);
1130
+ ReactNoop.render(<Foo ref={foo} b={0} />);
1131
+ expect(ReactNoop.flush()).toEqual(['A: 0, B: 0']);
1132
+
1133
+ a = 1;
1134
+ foo.current.forceUpdate();
1135
+ expect(ReactNoop.flush()).toEqual(['A: 1, B: 0']);
1136
+
1137
+ ReactNoop.render(<Foo ref={foo} b={0} />);
1138
+ expect(ReactNoop.flush()).toEqual([]);
1139
+ });
1140
+
1141
xit('can call sCU while resuming a partly mounted component', () => {
1142
let ops = [];
1143