Revert Rerender Error Check (#17519)
* Add failing test * Revert "Move rerender error check to avoid global state" This reverts commit 3e77742d8c4e64b89f816c0b1ce0bc156f8c5f61.
Sebastian Markbåge committed
Dec 3, 2019 at 23:10 UTC
5064c7f6aa2b46469ac601cc851640e91ec340a9
2 files changed
+76
-41
packages/react-reconciler/src/ReactFiberHooks.js
+45
-41
@@ -198,7 +198,8 @@ let renderPhaseUpdates: Map<
198
UpdateQueue<any, any>,
199
Update<any, any>,
200
> | null = null;
201
-
201
+// Counter to prevent infinite loops.
202
+let numberOfReRenders: number = 0;
203
const RE_RENDER_LIMIT = 25;
204
205
// In DEV, this is the name of the currently executing primitive hook
@@ -397,6 +398,7 @@ export function renderWithHooks(
398
399
// didScheduleRenderPhaseUpdate = false;
400
// renderPhaseUpdates = null;
401
+ // numberOfReRenders = 0;
402
403
// TODO Warn if no hooks are used at all during mount, then some are used during update.
404
// Currently we will identify the update render as a mount because memoizedState === null.
@@ -428,17 +430,8 @@ export function renderWithHooks(
430
let children = Component(props, refOrContext);
431
432
if (didScheduleRenderPhaseUpdate) {
431
- // Counter to prevent infinite loops.
432
- let numberOfReRenders: number = 0;
433
do {
434
didScheduleRenderPhaseUpdate = false;
435
-
436
- invariant(
437
- numberOfReRenders < RE_RENDER_LIMIT,
438
- 'Too many re-renders. React limits the number of renders to prevent ' +
439
- 'an infinite loop.',
440
- );
441
-
435
numberOfReRenders += 1;
436
if (__DEV__) {
437
// Even when hot reloading, allow dependencies to stabilize
@@ -465,6 +458,7 @@ export function renderWithHooks(
458
} while (didScheduleRenderPhaseUpdate);
459
460
renderPhaseUpdates = null;
461
+ numberOfReRenders = 0;
462
}
463
464
// We can assume the previous dispatcher is always this one, since we set it
@@ -495,6 +489,7 @@ export function renderWithHooks(
489
// These were reset above
490
// didScheduleRenderPhaseUpdate = false;
491
// renderPhaseUpdates = null;
492
+ // numberOfReRenders = 0;
493
494
invariant(
495
!didRenderTooFewHooks,
@@ -541,6 +536,7 @@ export function resetHooks(): void {
536
537
didScheduleRenderPhaseUpdate = false;
538
renderPhaseUpdates = null;
539
+ numberOfReRenders = 0;
540
}
541
542
function mountWorkInProgressHook(): Hook {
@@ -676,43 +672,45 @@ function updateReducer<S, I, A>(
672
673
queue.lastRenderedReducer = reducer;
674
679
- if (renderPhaseUpdates !== null) {
675
+ if (numberOfReRenders > 0) {
676
// This is a re-render. Apply the new render phase updates to the previous
677
// work-in-progress hook.
678
const dispatch: Dispatch<A> = (queue.dispatch: any);
683
- // Render phase updates are stored in a map of queue -> linked list
684
- const firstRenderPhaseUpdate = renderPhaseUpdates.get(queue);
685
- if (firstRenderPhaseUpdate !== undefined) {
686
- renderPhaseUpdates.delete(queue);
687
- let newState = hook.memoizedState;
688
- let update = firstRenderPhaseUpdate;
689
- do {
690
- // Process this render phase update. We don't have to check the
691
- // priority because it will always be the same as the current
692
- // render's.
693
- const action = update.action;
694
- newState = reducer(newState, action);
695
- update = update.next;
696
- } while (update !== null);
697
-
698
- // Mark that the fiber performed work, but only if the new state is
699
- // different from the current state.
700
- if (!is(newState, hook.memoizedState)) {
701
- markWorkInProgressReceivedUpdate();
702
- }
679
+ if (renderPhaseUpdates !== null) {
680
+ // Render phase updates are stored in a map of queue -> linked list
681
+ const firstRenderPhaseUpdate = renderPhaseUpdates.get(queue);
682
+ if (firstRenderPhaseUpdate !== undefined) {
683
+ renderPhaseUpdates.delete(queue);
684
+ let newState = hook.memoizedState;
685
+ let update = firstRenderPhaseUpdate;
686
+ do {
687
+ // Process this render phase update. We don't have to check the
688
+ // priority because it will always be the same as the current
689
+ // render's.
690
+ const action = update.action;
691
+ newState = reducer(newState, action);
692
+ update = update.next;
693
+ } while (update !== null);
694
704
- hook.memoizedState = newState;
705
- // Don't persist the state accumulated from the render phase updates to
706
- // the base state unless the queue is empty.
707
- // TODO: Not sure if this is the desired semantics, but it's what we
708
- // do for gDSFP. I can't remember why.
709
- if (hook.baseUpdate === queue.last) {
710
- hook.baseState = newState;
711
- }
695
+ // Mark that the fiber performed work, but only if the new state is
696
+ // different from the current state.
697
+ if (!is(newState, hook.memoizedState)) {
698
+ markWorkInProgressReceivedUpdate();
699
+ }
700
+
701
+ hook.memoizedState = newState;
702
+ // Don't persist the state accumulated from the render phase updates to
703
+ // the base state unless the queue is empty.
704
+ // TODO: Not sure if this is the desired semantics, but it's what we
705
+ // do for gDSFP. I can't remember why.
706
+ if (hook.baseUpdate === queue.last) {
707
+ hook.baseState = newState;
708
+ }
709
713
- queue.lastRenderedState = newState;
710
+ queue.lastRenderedState = newState;
711
715
- return [newState, dispatch];
712
+ return [newState, dispatch];
713
+ }
714
}
715
return [hook.memoizedState, dispatch];
716
}
@@ -1205,6 +1203,12 @@ function dispatchAction<S, A>(
1203
queue: UpdateQueue<S, A>,
1204
action: A,
1205
) {
1206
+ invariant(
1207
+ numberOfReRenders < RE_RENDER_LIMIT,
1208
+ 'Too many re-renders. React limits the number of renders to prevent ' +
1209
+ 'an infinite loop.',
1210
+ );
1211
+
1212
if (__DEV__) {
1213
warning(
1214
typeof arguments[3] !== 'function',
packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
+31
@@ -2493,4 +2493,35 @@ describe('ReactHooksWithNoopRenderer', () => {
2493
expect(Scheduler).toHaveYielded(['Step: 5, Shadow: 5']);
2494
expect(ReactNoop).toMatchRenderedOutput('5');
2495
});
2496
+
2497
+ it('should process the rest pending updates after a render phase update', () => {
2498
+ // Similar to previous test, except using a preceding render phase update
2499
+ // instead of new props.
2500
+ let updateA;
2501
+ let updateC;
2502
+ function App() {
2503
+ const [a, setA] = useState(false);
2504
+ const [b, setB] = useState(false);
2505
+ if (a !== b) {
2506
+ setB(a);
2507
+ }
2508
+ // Even though we called setB above,
2509
+ // we should still apply the changes to C,
2510
+ // during this render pass.
2511
+ const [c, setC] = useState(false);
2512
+ updateA = setA;
2513
+ updateC = setC;
2514
+ return `${a ? 'A' : 'a'}${b ? 'B' : 'b'}${c ? 'C' : 'c'}`;
2515
+ }
2516
+
2517
+ act(() => ReactNoop.render(<App />));
2518
+ expect(ReactNoop).toMatchRenderedOutput('abc');
2519
+
2520
+ act(() => {
2521
+ updateA(true);
2522
+ // This update should not get dropped.
2523
+ updateC(true);
2524
+ });
2525
+ expect(ReactNoop).toMatchRenderedOutput('ABC');
2526
+ });
2527
});