From 5064c7f6aa2b46469ac601cc851640e91ec340a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Tue, 3 Dec 2019 23:10:36 -0800 Subject: [PATCH] Revert Rerender Error Check (#17519) * Add failing test * Revert "Move rerender error check to avoid global state" This reverts commit 3e77742d8c4e64b89f816c0b1ce0bc156f8c5f61. --- .../react-reconciler/src/ReactFiberHooks.js | 86 ++++++++++--------- ...eactHooksWithNoopRenderer-test.internal.js | 31 +++++++ 2 files changed, 76 insertions(+), 41 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index bed33089415fc..7a4c7dfae82b7 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -198,7 +198,8 @@ let renderPhaseUpdates: Map< UpdateQueue, Update, > | null = null; - +// Counter to prevent infinite loops. +let numberOfReRenders: number = 0; const RE_RENDER_LIMIT = 25; // In DEV, this is the name of the currently executing primitive hook @@ -397,6 +398,7 @@ export function renderWithHooks( // didScheduleRenderPhaseUpdate = false; // renderPhaseUpdates = null; + // numberOfReRenders = 0; // TODO Warn if no hooks are used at all during mount, then some are used during update. // Currently we will identify the update render as a mount because memoizedState === null. @@ -428,17 +430,8 @@ export function renderWithHooks( let children = Component(props, refOrContext); if (didScheduleRenderPhaseUpdate) { - // Counter to prevent infinite loops. - let numberOfReRenders: number = 0; do { didScheduleRenderPhaseUpdate = false; - - invariant( - numberOfReRenders < RE_RENDER_LIMIT, - 'Too many re-renders. React limits the number of renders to prevent ' + - 'an infinite loop.', - ); - numberOfReRenders += 1; if (__DEV__) { // Even when hot reloading, allow dependencies to stabilize @@ -465,6 +458,7 @@ export function renderWithHooks( } while (didScheduleRenderPhaseUpdate); renderPhaseUpdates = null; + numberOfReRenders = 0; } // We can assume the previous dispatcher is always this one, since we set it @@ -495,6 +489,7 @@ export function renderWithHooks( // These were reset above // didScheduleRenderPhaseUpdate = false; // renderPhaseUpdates = null; + // numberOfReRenders = 0; invariant( !didRenderTooFewHooks, @@ -541,6 +536,7 @@ export function resetHooks(): void { didScheduleRenderPhaseUpdate = false; renderPhaseUpdates = null; + numberOfReRenders = 0; } function mountWorkInProgressHook(): Hook { @@ -676,43 +672,45 @@ function updateReducer( queue.lastRenderedReducer = reducer; - if (renderPhaseUpdates !== null) { + if (numberOfReRenders > 0) { // This is a re-render. Apply the new render phase updates to the previous // work-in-progress hook. const dispatch: Dispatch = (queue.dispatch: any); - // Render phase updates are stored in a map of queue -> linked list - const firstRenderPhaseUpdate = renderPhaseUpdates.get(queue); - if (firstRenderPhaseUpdate !== undefined) { - renderPhaseUpdates.delete(queue); - let newState = hook.memoizedState; - let update = firstRenderPhaseUpdate; - do { - // Process this render phase update. We don't have to check the - // priority because it will always be the same as the current - // render's. - const action = update.action; - newState = reducer(newState, action); - update = update.next; - } while (update !== null); - - // Mark that the fiber performed work, but only if the new state is - // different from the current state. - if (!is(newState, hook.memoizedState)) { - markWorkInProgressReceivedUpdate(); - } + if (renderPhaseUpdates !== null) { + // Render phase updates are stored in a map of queue -> linked list + const firstRenderPhaseUpdate = renderPhaseUpdates.get(queue); + if (firstRenderPhaseUpdate !== undefined) { + renderPhaseUpdates.delete(queue); + let newState = hook.memoizedState; + let update = firstRenderPhaseUpdate; + do { + // Process this render phase update. We don't have to check the + // priority because it will always be the same as the current + // render's. + const action = update.action; + newState = reducer(newState, action); + update = update.next; + } while (update !== null); - hook.memoizedState = newState; - // Don't persist the state accumulated from the render phase updates to - // the base state unless the queue is empty. - // TODO: Not sure if this is the desired semantics, but it's what we - // do for gDSFP. I can't remember why. - if (hook.baseUpdate === queue.last) { - hook.baseState = newState; - } + // Mark that the fiber performed work, but only if the new state is + // different from the current state. + if (!is(newState, hook.memoizedState)) { + markWorkInProgressReceivedUpdate(); + } + + hook.memoizedState = newState; + // Don't persist the state accumulated from the render phase updates to + // the base state unless the queue is empty. + // TODO: Not sure if this is the desired semantics, but it's what we + // do for gDSFP. I can't remember why. + if (hook.baseUpdate === queue.last) { + hook.baseState = newState; + } - queue.lastRenderedState = newState; + queue.lastRenderedState = newState; - return [newState, dispatch]; + return [newState, dispatch]; + } } return [hook.memoizedState, dispatch]; } @@ -1205,6 +1203,12 @@ function dispatchAction( queue: UpdateQueue, action: A, ) { + invariant( + numberOfReRenders < RE_RENDER_LIMIT, + 'Too many re-renders. React limits the number of renders to prevent ' + + 'an infinite loop.', + ); + if (__DEV__) { warning( typeof arguments[3] !== 'function', diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index c6d5e5c81b748..5e60e235ac434 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -2493,4 +2493,35 @@ describe('ReactHooksWithNoopRenderer', () => { expect(Scheduler).toHaveYielded(['Step: 5, Shadow: 5']); expect(ReactNoop).toMatchRenderedOutput('5'); }); + + it('should process the rest pending updates after a render phase update', () => { + // Similar to previous test, except using a preceding render phase update + // instead of new props. + let updateA; + let updateC; + function App() { + const [a, setA] = useState(false); + const [b, setB] = useState(false); + if (a !== b) { + setB(a); + } + // Even though we called setB above, + // we should still apply the changes to C, + // during this render pass. + const [c, setC] = useState(false); + updateA = setA; + updateC = setC; + return `${a ? 'A' : 'a'}${b ? 'B' : 'b'}${c ? 'C' : 'c'}`; + } + + act(() => ReactNoop.render()); + expect(ReactNoop).toMatchRenderedOutput('abc'); + + act(() => { + updateA(true); + // This update should not get dropped. + updateC(true); + }); + expect(ReactNoop).toMatchRenderedOutput('ABC'); + }); });