From f14679f754bc71d15fa09b4f530f33cf82c6acc2 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Fri, 13 Dec 2019 12:53:32 -0800 Subject: [PATCH 1/3] Render phase updates can now be extracted from the pending queue --- .../react-reconciler/src/ReactFiberHooks.js | 167 +++++++----------- 1 file changed, 67 insertions(+), 100 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index d2c79fc28096..fef21698ee34 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -193,12 +193,9 @@ 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. +// TODO: This is only global so that we can detect whether we're currently +// re-rendering. Use a custom dispatcher instead and make this local. let numberOfReRenders: number = 0; const RE_RENDER_LIMIT = 25; @@ -393,7 +390,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. @@ -453,7 +449,6 @@ export function renderWithHooks( children = Component(props, refOrContext); } while (didScheduleRenderPhaseUpdate); - renderPhaseUpdates = null; numberOfReRenders = 0; } @@ -484,7 +479,6 @@ export function renderWithHooks( // These were reset above // didScheduleRenderPhaseUpdate = false; - // renderPhaseUpdates = null; // numberOfReRenders = 0; invariant( @@ -531,7 +525,6 @@ export function resetHooks(): void { } didScheduleRenderPhaseUpdate = false; - renderPhaseUpdates = null; numberOfReRenders = 0; } @@ -672,43 +665,41 @@ function updateReducer( // 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; + 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(); + } - return [newState, dispatch]; + 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 [hook.memoizedState, dispatch]; + return [newState, dispatch]; } const current: Hook = (currentHook: any); @@ -1243,6 +1234,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 +1275,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) From 63880405a397280a326353c3a36c260c7a0457b8 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Fri, 13 Dec 2019 13:55:30 -0800 Subject: [PATCH 2/3] Use a custom dispatcher for the second render pass --- .../react-reconciler/src/ReactFiberHooks.js | 398 +++++++++++++++--- 1 file changed, 340 insertions(+), 58 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index fef21698ee34..6444d9746e1e 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -193,10 +193,7 @@ let workInProgressHook: Hook | null = null; // Whether an update was scheduled during the currently executing render pass. let didScheduleRenderPhaseUpdate: boolean = false; -// Counter to prevent infinite loops. -// TODO: This is only global so that we can detect whether we're currently -// re-rendering. Use a custom dispatcher instead and make this local. -let numberOfReRenders: number = 0; + const RE_RENDER_LIMIT = 25; // In DEV, this is the name of the currently executing primitive hook @@ -390,7 +387,6 @@ export function renderWithHooks( // workInProgressHook = null; // didScheduleRenderPhaseUpdate = false; - // 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. @@ -422,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 @@ -443,13 +448,11 @@ export function renderWithHooks( } ReactCurrentDispatcher.current = __DEV__ - ? HooksDispatcherOnUpdateInDEV - : HooksDispatcherOnUpdate; + ? HooksDispatcherOnRerenderInDEV + : HooksDispatcherOnRerender; children = Component(props, refOrContext); } while (didScheduleRenderPhaseUpdate); - - numberOfReRenders = 0; } // We can assume the previous dispatcher is always this one, since we set it @@ -479,7 +482,6 @@ export function renderWithHooks( // These were reset above // didScheduleRenderPhaseUpdate = false; - // numberOfReRenders = 0; invariant( !didRenderTooFewHooks, @@ -525,7 +527,6 @@ export function resetHooks(): void { } didScheduleRenderPhaseUpdate = false; - numberOfReRenders = 0; } function mountWorkInProgressHook(): Hook { @@ -661,47 +662,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); - 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]; - } - const current: Hook = (currentHook: any); // The last rebase update that is NOT part of the base state. @@ -819,6 +779,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>] { @@ -849,6 +863,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, @@ -1218,12 +1238,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( @@ -1381,11 +1395,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 = () => { @@ -1762,6 +1796,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, @@ -2023,4 +2174,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); + }, + }; } From b384dd106027cd5cecee06ddf3e3d52a9737ba9c Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 13 Dec 2019 16:26:00 -0800 Subject: [PATCH 3/3] 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 | 25 +++++++++-- .../src/ReactFiberWorkLoop.js | 6 +-- ...eactHooksWithNoopRenderer-test.internal.js | 44 +++++++++++++++++++ 4 files changed, 71 insertions(+), 9 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 6444d9746e1e..461db6649881 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -504,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); 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', () => {