From 8a347ed024159a5307172ea633a4561160f4a6b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Fri, 13 Dec 2019 16:45:09 -0800 Subject: [PATCH] Remove renderPhaseUpdates Map (#17484) * Render phase updates can now be extracted from the pending queue * Use a custom dispatcher for the second render pass * Discard render phase updates if component throws When aborting a render, we also need to throw out render phase updates. Remove the updates from the queues so they do not persist to the next render. We already did a single pass through the whole list of hooks, so we know that any pending updates must have been dispatched during the render phase. The ones that were dispatched before we started rendering were already transferred to the current hook's queue. --- .../src/ReactFiberBeginWork.js | 5 +- .../react-reconciler/src/ReactFiberHooks.js | 522 +++++++++++++----- .../src/ReactFiberWorkLoop.js | 6 +- ...eactHooksWithNoopRenderer-test.internal.js | 44 ++ 4 files changed, 444 insertions(+), 133 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index ada9a09c2a30..b08602d09cc6 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -136,7 +136,7 @@ import { calculateChangedBits, scheduleWorkOnParentPath, } from './ReactFiberNewContext'; -import {resetHooks, renderWithHooks, bailoutHooks} from './ReactFiberHooks'; +import {renderWithHooks, bailoutHooks} from './ReactFiberHooks'; import {stopProfilerTimerIfRunning} from './ReactProfilerTimer'; import { getMaskedContext, @@ -1319,7 +1319,8 @@ function mountIndeterminateComponent( workInProgress.tag = ClassComponent; // Throw out any hooks that were used. - resetHooks(); + workInProgress.memoizedState = null; + workInProgress.updateQueue = null; // Push context providers early to prevent context stack mismatches. // During mounting we don't know the child context yet as the instance doesn't exist. diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index d2c79fc28096..461db6649881 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -193,13 +193,7 @@ let workInProgressHook: Hook | null = null; // Whether an update was scheduled during the currently executing render pass. let didScheduleRenderPhaseUpdate: boolean = false; -// Lazily created map of render-phase updates -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 @@ -393,8 +387,6 @@ export function renderWithHooks( // workInProgressHook = null; // 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. @@ -426,8 +418,17 @@ 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 @@ -447,14 +448,11 @@ export function renderWithHooks( } ReactCurrentDispatcher.current = __DEV__ - ? HooksDispatcherOnUpdateInDEV - : HooksDispatcherOnUpdate; + ? HooksDispatcherOnRerenderInDEV + : HooksDispatcherOnRerender; children = Component(props, refOrContext); } while (didScheduleRenderPhaseUpdate); - - renderPhaseUpdates = null; - numberOfReRenders = 0; } // We can assume the previous dispatcher is always this one, since we set it @@ -484,8 +482,6 @@ export function renderWithHooks( // These were reset above // didScheduleRenderPhaseUpdate = false; - // renderPhaseUpdates = null; - // numberOfReRenders = 0; invariant( !didRenderTooFewHooks, @@ -508,14 +504,31 @@ export function bailoutHooks( } } -export function resetHooks(): void { +export function resetHooksAfterThrow(): void { // We can assume the previous dispatcher is always this one, since we set it // at the beginning of the render phase and there's no re-entrancy. ReactCurrentDispatcher.current = ContextOnlyDispatcher; - // This is used to reset the state of this module when a component throws. - // It's also called inside mountIndeterminateComponent if we determine the - // component is a module-style component. + if (didScheduleRenderPhaseUpdate) { + const current = (currentlyRenderingFiber: any).alternate; + if (current !== null) { + // There were render phase updates. These are only valid for this render + // pass, which we are now aborting. Remove the updates from the queues so + // they do not persist to the next render. We already did a single pass + // through the whole list of hooks, so we know that any pending updates + // must have been dispatched during the render phase. The ones that were + // dispatched before we started rendering were already transferred to the + // current hook's queue. + let hook: Hook | null = current.memoizedState; + while (hook !== null) { + const queue = hook.queue; + if (queue !== null) { + queue.pending = null; + } + hook = hook.next; + } + } + } renderExpirationTime = NoWork; currentlyRenderingFiber = (null: any); @@ -531,8 +544,6 @@ export function resetHooks(): void { } didScheduleRenderPhaseUpdate = false; - renderPhaseUpdates = null; - numberOfReRenders = 0; } function mountWorkInProgressHook(): Hook { @@ -668,49 +679,6 @@ function updateReducer( queue.lastRenderedReducer = reducer; - 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); - 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); - - // 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.baseQueue === null) { - hook.baseState = newState; - } - - queue.lastRenderedState = newState; - - return [newState, dispatch]; - } - } - return [hook.memoizedState, dispatch]; - } - const current: Hook = (currentHook: any); // The last rebase update that is NOT part of the base state. @@ -828,6 +796,60 @@ function updateReducer( return [hook.memoizedState, dispatch]; } +function rerenderReducer( + reducer: (S, A) => S, + initialArg: I, + init?: I => S, +): [S, Dispatch] { + const hook = updateWorkInProgressHook(); + const queue = hook.queue; + invariant( + queue !== null, + 'Should have a queue. This is likely a bug in React. Please file an issue.', + ); + + queue.lastRenderedReducer = reducer; + + // This is a re-render. Apply the new render phase updates to the previous + // work-in-progress hook. + const dispatch: Dispatch = (queue.dispatch: any); + const lastRenderPhaseUpdate = queue.pending; + let newState = hook.memoizedState; + if (lastRenderPhaseUpdate !== null) { + // The queue doesn't persist past this render pass. + queue.pending = null; + + const firstRenderPhaseUpdate = lastRenderPhaseUpdate.next; + 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 !== firstRenderPhaseUpdate); + + // 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.baseQueue === null) { + hook.baseState = newState; + } + + queue.lastRenderedState = newState; + } + return [newState, dispatch]; +} + function mountState( initialState: (() => S) | S, ): [S, Dispatch>] { @@ -858,6 +880,12 @@ function updateState( return updateReducer(basicStateReducer, (initialState: any)); } +function rerenderState( + initialState: (() => S) | S, +): [S, Dispatch>] { + return rerenderReducer(basicStateReducer, (initialState: any)); +} + function pushEffect(tag, create, destroy, deps) { const effect: Effect = { tag, @@ -1227,12 +1255,6 @@ 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__) { if (typeof arguments[3] === 'function') { warning( @@ -1243,6 +1265,38 @@ function dispatchAction( } } + const currentTime = requestCurrentTimeForUpdate(); + const suspenseConfig = requestCurrentSuspenseConfig(); + const expirationTime = computeExpirationForFiber( + currentTime, + fiber, + suspenseConfig, + ); + + const update: Update = { + expirationTime, + suspenseConfig, + action, + eagerReducer: null, + eagerState: null, + next: (null: any), + }; + + if (__DEV__) { + update.priority = getCurrentPriorityLevel(); + } + + // Append the update to the end of the list. + const pending = queue.pending; + if (pending === null) { + // This is the first update. Create a circular list. + update.next = update; + } else { + update.next = pending.next; + pending.next = update; + } + queue.pending = update; + const alternate = fiber.alternate; if ( fiber === currentlyRenderingFiber || @@ -1252,64 +1306,8 @@ function dispatchAction( // queue -> linked list of updates. After this render pass, we'll restart // and apply the stashed updates on top of the work-in-progress hook. didScheduleRenderPhaseUpdate = true; - const update: Update = { - expirationTime: renderExpirationTime, - suspenseConfig: null, - action, - eagerReducer: null, - eagerState: null, - next: (null: any), - }; - if (__DEV__) { - update.priority = getCurrentPriorityLevel(); - } - if (renderPhaseUpdates === null) { - renderPhaseUpdates = new Map(); - } - const firstRenderPhaseUpdate = renderPhaseUpdates.get(queue); - if (firstRenderPhaseUpdate === undefined) { - renderPhaseUpdates.set(queue, update); - } else { - // Append the update to the end of the list. - let lastRenderPhaseUpdate = firstRenderPhaseUpdate; - while (lastRenderPhaseUpdate.next !== null) { - lastRenderPhaseUpdate = lastRenderPhaseUpdate.next; - } - lastRenderPhaseUpdate.next = update; - } + update.expirationTime = renderExpirationTime; } else { - const currentTime = requestCurrentTimeForUpdate(); - const suspenseConfig = requestCurrentSuspenseConfig(); - const expirationTime = computeExpirationForFiber( - currentTime, - fiber, - suspenseConfig, - ); - - const update: Update = { - expirationTime, - suspenseConfig, - action, - eagerReducer: null, - eagerState: null, - next: (null: any), - }; - - if (__DEV__) { - update.priority = getCurrentPriorityLevel(); - } - - // Append the update to the end of the list. - const pending = queue.pending; - if (pending === null) { - // This is the first update. Create a circular list. - update.next = update; - } else { - update.next = pending.next; - pending.next = update; - } - queue.pending = update; - if ( fiber.expirationTime === NoWork && (alternate === null || alternate.expirationTime === NoWork) @@ -1414,11 +1412,31 @@ const HooksDispatcherOnUpdate: Dispatcher = { useTransition: updateTransition, }; +const HooksDispatcherOnRerender: Dispatcher = { + readContext, + + useCallback: updateCallback, + useContext: readContext, + useEffect: updateEffect, + useImperativeHandle: updateImperativeHandle, + useLayoutEffect: updateLayoutEffect, + useMemo: updateMemo, + useReducer: rerenderReducer, + useRef: updateRef, + useState: rerenderState, + useDebugValue: updateDebugValue, + useResponder: createResponderListener, + useDeferredValue: updateDeferredValue, + useTransition: updateTransition, +}; + let HooksDispatcherOnMountInDEV: Dispatcher | null = null; let HooksDispatcherOnMountWithHookTypesInDEV: Dispatcher | null = null; let HooksDispatcherOnUpdateInDEV: Dispatcher | null = null; +let HooksDispatcherOnRerenderInDEV: Dispatcher | null = null; let InvalidNestedHooksDispatcherOnMountInDEV: Dispatcher | null = null; let InvalidNestedHooksDispatcherOnUpdateInDEV: Dispatcher | null = null; +let InvalidNestedHooksDispatcherOnRerenderInDEV: Dispatcher | null = null; if (__DEV__) { const warnInvalidContextAccess = () => { @@ -1795,6 +1813,123 @@ if (__DEV__) { }, }; + HooksDispatcherOnRerenderInDEV = { + readContext( + context: ReactContext, + observedBits: void | number | boolean, + ): T { + return readContext(context, observedBits); + }, + + useCallback(callback: T, deps: Array | void | null): T { + currentHookNameInDev = 'useCallback'; + updateHookTypesDev(); + return updateCallback(callback, deps); + }, + useContext( + context: ReactContext, + observedBits: void | number | boolean, + ): T { + currentHookNameInDev = 'useContext'; + updateHookTypesDev(); + return readContext(context, observedBits); + }, + useEffect( + create: () => (() => void) | void, + deps: Array | void | null, + ): void { + currentHookNameInDev = 'useEffect'; + updateHookTypesDev(); + return updateEffect(create, deps); + }, + useImperativeHandle( + ref: {current: T | null} | ((inst: T | null) => mixed) | null | void, + create: () => T, + deps: Array | void | null, + ): void { + currentHookNameInDev = 'useImperativeHandle'; + updateHookTypesDev(); + return updateImperativeHandle(ref, create, deps); + }, + useLayoutEffect( + create: () => (() => void) | void, + deps: Array | void | null, + ): void { + currentHookNameInDev = 'useLayoutEffect'; + updateHookTypesDev(); + return updateLayoutEffect(create, deps); + }, + useMemo(create: () => T, deps: Array | void | null): T { + currentHookNameInDev = 'useMemo'; + updateHookTypesDev(); + const prevDispatcher = ReactCurrentDispatcher.current; + ReactCurrentDispatcher.current = InvalidNestedHooksDispatcherOnRerenderInDEV; + try { + return updateMemo(create, deps); + } finally { + ReactCurrentDispatcher.current = prevDispatcher; + } + }, + useReducer( + reducer: (S, A) => S, + initialArg: I, + init?: I => S, + ): [S, Dispatch] { + currentHookNameInDev = 'useReducer'; + updateHookTypesDev(); + const prevDispatcher = ReactCurrentDispatcher.current; + ReactCurrentDispatcher.current = InvalidNestedHooksDispatcherOnRerenderInDEV; + try { + return rerenderReducer(reducer, initialArg, init); + } finally { + ReactCurrentDispatcher.current = prevDispatcher; + } + }, + useRef(initialValue: T): {current: T} { + currentHookNameInDev = 'useRef'; + updateHookTypesDev(); + return updateRef(initialValue); + }, + useState( + initialState: (() => S) | S, + ): [S, Dispatch>] { + currentHookNameInDev = 'useState'; + updateHookTypesDev(); + const prevDispatcher = ReactCurrentDispatcher.current; + ReactCurrentDispatcher.current = InvalidNestedHooksDispatcherOnRerenderInDEV; + try { + return rerenderState(initialState); + } finally { + ReactCurrentDispatcher.current = prevDispatcher; + } + }, + useDebugValue(value: T, formatterFn: ?(value: T) => mixed): void { + currentHookNameInDev = 'useDebugValue'; + updateHookTypesDev(); + return updateDebugValue(value, formatterFn); + }, + useResponder( + responder: ReactEventResponder, + props, + ): ReactEventResponderListener { + currentHookNameInDev = 'useResponder'; + updateHookTypesDev(); + return createResponderListener(responder, props); + }, + useDeferredValue(value: T, config: TimeoutConfig | void | null): T { + currentHookNameInDev = 'useDeferredValue'; + updateHookTypesDev(); + return updateDeferredValue(value, config); + }, + useTransition( + config: SuspenseConfig | void | null, + ): [(() => void) => void, boolean] { + currentHookNameInDev = 'useTransition'; + updateHookTypesDev(); + return updateTransition(config); + }, + }; + InvalidNestedHooksDispatcherOnMountInDEV = { readContext( context: ReactContext, @@ -2056,4 +2191,135 @@ if (__DEV__) { return updateTransition(config); }, }; + + InvalidNestedHooksDispatcherOnRerenderInDEV = { + readContext( + context: ReactContext, + observedBits: void | number | boolean, + ): T { + warnInvalidContextAccess(); + return readContext(context, observedBits); + }, + + useCallback(callback: T, deps: Array | void | null): T { + currentHookNameInDev = 'useCallback'; + warnInvalidHookAccess(); + updateHookTypesDev(); + return updateCallback(callback, deps); + }, + useContext( + context: ReactContext, + observedBits: void | number | boolean, + ): T { + currentHookNameInDev = 'useContext'; + warnInvalidHookAccess(); + updateHookTypesDev(); + return readContext(context, observedBits); + }, + useEffect( + create: () => (() => void) | void, + deps: Array | void | null, + ): void { + currentHookNameInDev = 'useEffect'; + warnInvalidHookAccess(); + updateHookTypesDev(); + return updateEffect(create, deps); + }, + useImperativeHandle( + ref: {current: T | null} | ((inst: T | null) => mixed) | null | void, + create: () => T, + deps: Array | void | null, + ): void { + currentHookNameInDev = 'useImperativeHandle'; + warnInvalidHookAccess(); + updateHookTypesDev(); + return updateImperativeHandle(ref, create, deps); + }, + useLayoutEffect( + create: () => (() => void) | void, + deps: Array | void | null, + ): void { + currentHookNameInDev = 'useLayoutEffect'; + warnInvalidHookAccess(); + updateHookTypesDev(); + return updateLayoutEffect(create, deps); + }, + useMemo(create: () => T, deps: Array | void | null): T { + currentHookNameInDev = 'useMemo'; + warnInvalidHookAccess(); + updateHookTypesDev(); + const prevDispatcher = ReactCurrentDispatcher.current; + ReactCurrentDispatcher.current = InvalidNestedHooksDispatcherOnUpdateInDEV; + try { + return updateMemo(create, deps); + } finally { + ReactCurrentDispatcher.current = prevDispatcher; + } + }, + useReducer( + reducer: (S, A) => S, + initialArg: I, + init?: I => S, + ): [S, Dispatch] { + currentHookNameInDev = 'useReducer'; + warnInvalidHookAccess(); + updateHookTypesDev(); + const prevDispatcher = ReactCurrentDispatcher.current; + ReactCurrentDispatcher.current = InvalidNestedHooksDispatcherOnUpdateInDEV; + try { + return rerenderReducer(reducer, initialArg, init); + } finally { + ReactCurrentDispatcher.current = prevDispatcher; + } + }, + useRef(initialValue: T): {current: T} { + currentHookNameInDev = 'useRef'; + warnInvalidHookAccess(); + updateHookTypesDev(); + return updateRef(initialValue); + }, + useState( + initialState: (() => S) | S, + ): [S, Dispatch>] { + currentHookNameInDev = 'useState'; + warnInvalidHookAccess(); + updateHookTypesDev(); + const prevDispatcher = ReactCurrentDispatcher.current; + ReactCurrentDispatcher.current = InvalidNestedHooksDispatcherOnUpdateInDEV; + try { + return rerenderState(initialState); + } finally { + ReactCurrentDispatcher.current = prevDispatcher; + } + }, + useDebugValue(value: T, formatterFn: ?(value: T) => mixed): void { + currentHookNameInDev = 'useDebugValue'; + warnInvalidHookAccess(); + updateHookTypesDev(); + return updateDebugValue(value, formatterFn); + }, + useResponder( + responder: ReactEventResponder, + props, + ): ReactEventResponderListener { + currentHookNameInDev = 'useResponder'; + warnInvalidHookAccess(); + updateHookTypesDev(); + return createResponderListener(responder, props); + }, + useDeferredValue(value: T, config: TimeoutConfig | void | null): T { + currentHookNameInDev = 'useDeferredValue'; + warnInvalidHookAccess(); + updateHookTypesDev(); + return updateDeferredValue(value, config); + }, + useTransition( + config: SuspenseConfig | void | null, + ): [(() => void) => void, boolean] { + currentHookNameInDev = 'useTransition'; + warnInvalidHookAccess(); + updateHookTypesDev(); + return updateTransition(config); + }, + }; } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 81ad6e77f3dc..10d4eeffcb19 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -140,7 +140,7 @@ import { } from './ReactFiberCommitWork'; import {enqueueUpdate} from './ReactUpdateQueue'; import {resetContextDependencies} from './ReactFiberNewContext'; -import {resetHooks, ContextOnlyDispatcher} from './ReactFiberHooks'; +import {resetHooksAfterThrow, ContextOnlyDispatcher} from './ReactFiberHooks'; import {createCapturedValue} from './ReactCapturedValue'; import { @@ -1281,7 +1281,7 @@ function handleError(root, thrownValue) { try { // Reset module-level state that was set during the render phase. resetContextDependencies(); - resetHooks(); + resetHooksAfterThrow(); resetCurrentDebugFiberInDEV(); if (workInProgress === null || workInProgress.return === null) { @@ -2635,7 +2635,7 @@ if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) { // Keep this code in sync with handleError; any changes here must have // corresponding changes there. resetContextDependencies(); - resetHooks(); + resetHooksAfterThrow(); // Don't reset current debug fiber, since we're about to work on the // same fiber again. diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index 4b13251a2de0..752a6ed0a105 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -542,6 +542,50 @@ describe('ReactHooksWithNoopRenderer', () => { ]); expect(ReactNoop.getChildren()).toEqual([span(22)]); }); + + it('discards render phase updates if something suspends', () => { + const thenable = {then() {}}; + function Foo({signal}) { + return ( + + + + ); + } + + function Bar({signal: newSignal}) { + let [counter, setCounter] = useState(0); + let [signal, setSignal] = useState(true); + + // Increment a counter every time the signal changes + if (signal !== newSignal) { + setCounter(c => c + 1); + setSignal(newSignal); + if (counter === 0) { + // We're suspending during a render that includes render phase + // updates. Those updates should not persist to the next render. + Scheduler.unstable_yieldValue('Suspend!'); + throw thenable; + } + } + + return ; + } + + const root = ReactNoop.createRoot(); + root.render(); + + expect(Scheduler).toFlushAndYield([0]); + expect(root).toMatchRenderedOutput(); + + root.render(); + expect(Scheduler).toFlushAndYield(['Suspend!']); + expect(root).toMatchRenderedOutput(); + + // Rendering again should suspend again. + root.render(); + expect(Scheduler).toFlushAndYield(['Suspend!']); + }); }); describe('useReducer', () => {