From 6807fdea3d273f2098c0a2e5b4b755a8d7f5b1b5 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 16 Dec 2019 18:42:58 +0000 Subject: [PATCH] Revert "Remove renderPhaseUpdates Map (#17484)" This reverts commit 8a347ed024159a5307172ea633a4561160f4a6b9. --- .../src/ReactFiberBeginWork.js | 5 +- .../react-reconciler/src/ReactFiberHooks.js | 522 +++++------------- .../src/ReactFiberWorkLoop.js | 6 +- ...eactHooksWithNoopRenderer-test.internal.js | 44 -- 4 files changed, 133 insertions(+), 444 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 554aee6693b9..dddf6984ce24 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -135,7 +135,7 @@ import { calculateChangedBits, scheduleWorkOnParentPath, } from './ReactFiberNewContext'; -import {renderWithHooks, bailoutHooks} from './ReactFiberHooks'; +import {resetHooks, renderWithHooks, bailoutHooks} from './ReactFiberHooks'; import {stopProfilerTimerIfRunning} from './ReactProfilerTimer'; import { getMaskedContext, @@ -1318,8 +1318,7 @@ function mountIndeterminateComponent( workInProgress.tag = ClassComponent; // Throw out any hooks that were used. - workInProgress.memoizedState = null; - workInProgress.updateQueue = null; + resetHooks(); // 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 bed592711050..530ddc193fcb 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -192,7 +192,13 @@ 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 @@ -386,6 +392,8 @@ 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. @@ -417,17 +425,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 @@ -447,11 +446,14 @@ export function renderWithHooks( } ReactCurrentDispatcher.current = __DEV__ - ? HooksDispatcherOnRerenderInDEV - : HooksDispatcherOnRerender; + ? HooksDispatcherOnUpdateInDEV + : HooksDispatcherOnUpdate; children = Component(props, refOrContext); } while (didScheduleRenderPhaseUpdate); + + renderPhaseUpdates = null; + numberOfReRenders = 0; } // We can assume the previous dispatcher is always this one, since we set it @@ -481,6 +483,8 @@ export function renderWithHooks( // These were reset above // didScheduleRenderPhaseUpdate = false; + // renderPhaseUpdates = null; + // numberOfReRenders = 0; invariant( !didRenderTooFewHooks, @@ -503,31 +507,14 @@ export function bailoutHooks( } } -export function resetHooksAfterThrow(): void { +export function resetHooks(): 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; - 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; - } - } - } + // 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. renderExpirationTime = NoWork; currentlyRenderingFiber = (null: any); @@ -543,6 +530,8 @@ export function resetHooksAfterThrow(): void { } didScheduleRenderPhaseUpdate = false; + renderPhaseUpdates = null; + numberOfReRenders = 0; } function mountWorkInProgressHook(): Hook { @@ -678,6 +667,49 @@ 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. @@ -795,60 +827,6 @@ 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>] { @@ -879,12 +857,6 @@ 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, @@ -1254,6 +1226,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__) { if (typeof arguments[3] === 'function') { console.error( @@ -1264,38 +1242,6 @@ 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 || @@ -1305,8 +1251,64 @@ 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; - update.expirationTime = renderExpirationTime; + 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; + } } 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) @@ -1411,31 +1413,11 @@ 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 = () => { @@ -1812,123 +1794,6 @@ 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, @@ -2190,135 +2055,4 @@ 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 758bc50f2780..bb027d93a44a 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 {resetHooksAfterThrow, ContextOnlyDispatcher} from './ReactFiberHooks'; +import {resetHooks, ContextOnlyDispatcher} from './ReactFiberHooks'; import {createCapturedValue} from './ReactCapturedValue'; import { @@ -1280,7 +1280,7 @@ function handleError(root, thrownValue) { try { // Reset module-level state that was set during the render phase. resetContextDependencies(); - resetHooksAfterThrow(); + resetHooks(); resetCurrentDebugFiberInDEV(); if (workInProgress === null || workInProgress.return === null) { @@ -2634,7 +2634,7 @@ if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) { // Keep this code in sync with handleError; any changes here must have // corresponding changes there. resetContextDependencies(); - resetHooksAfterThrow(); + resetHooks(); // 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 41d237f30ac6..fc8f1661a382 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -542,50 +542,6 @@ 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', () => {