From 95bd7aad7daa80c381faa3215c80b0906ab5ead5 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 17 Jan 2020 16:00:35 -0500 Subject: [PATCH] Remove renderPhaseUpdates Map (#17625) * Remove renderPhaseUpdates Map Follow up to #17484, which was reverted due to a bug found in www. * Failing test: Dropped updates When resetting render phase updates after a throw, we should only clear the pending queue of hooks that were already processed. * Fix non-render-phase updates being dropped Detects if a queue has been processed by whether the hook was cloned. If we change the implementation to an array instead of a list, we'll need some other mechanism to determine whether the hook was processed. * Regression test: startTransition in render phase useTransition uses the state hook as part of its implementation, so we need to fork it in the dispatcher used for re-renders, too. --- .../src/ReactFiberBeginWork.js | 5 +- .../react-reconciler/src/ReactFiberHooks.js | 573 +++++++++++++----- .../src/ReactFiberWorkLoop.js | 6 +- ...eactHooksWithNoopRenderer-test.internal.js | 126 ++++ 4 files changed, 563 insertions(+), 147 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 6c34f2bbf4c6..9db5886a6c2b 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -137,7 +137,7 @@ import { calculateChangedBits, scheduleWorkOnParentPath, } from './ReactFiberNewContext'; -import {resetHooks, renderWithHooks, bailoutHooks} from './ReactFiberHooks'; +import {renderWithHooks, bailoutHooks} from './ReactFiberHooks'; import {stopProfilerTimerIfRunning} from './ReactProfilerTimer'; import { getMaskedContext, @@ -1407,7 +1407,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 393b2c565366..6022e54a138d 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -177,23 +177,12 @@ let currentlyRenderingFiber: Fiber = (null: any); let currentHook: Hook | null = null; let workInProgressHook: Hook | null = null; -// Updates scheduled during render will trigger an immediate re-render at the -// end of the current pass. We can't store these updates on the normal queue, -// because if the work is aborted, they should be discarded. Because this is -// a relatively rare case, we also don't want to add an additional field to -// either the hook or queue object types. So we store them in a lazily create -// map of queue -> render-phase updates, which are discarded once the component -// completes without re-rendering. - -// Whether an update was scheduled during the currently executing render pass. +// Whether an update was scheduled at any point during the render phase. This +// does not get reset if we do another render pass; only when we're completely +// finished evaluating this component. This is an optimization so we know +// whether we need to clear render phase updates after a throw. 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 @@ -387,8 +376,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. @@ -419,9 +406,20 @@ export function renderWithHooks( let children = Component(props, secondArg); - if (didScheduleRenderPhaseUpdate) { + // Check if there was a render phase update + if (workInProgress.expirationTime === renderExpirationTime) { + // Keep rendering in a loop for as long as render phase updates continue to + // be scheduled. Use a counter to prevent infinite loops. + let numberOfReRenders: number = 0; do { - didScheduleRenderPhaseUpdate = false; + workInProgress.expirationTime = NoWork; + + 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 @@ -441,14 +439,11 @@ export function renderWithHooks( } ReactCurrentDispatcher.current = __DEV__ - ? HooksDispatcherOnUpdateInDEV - : HooksDispatcherOnUpdate; + ? HooksDispatcherOnRerenderInDEV + : HooksDispatcherOnRerender; children = Component(props, secondArg); - } while (didScheduleRenderPhaseUpdate); - - renderPhaseUpdates = null; - numberOfReRenders = 0; + } while (workInProgress.expirationTime === renderExpirationTime); } // We can assume the previous dispatcher is always this one, since we set it @@ -476,10 +471,7 @@ export function renderWithHooks( hookTypesUpdateIndexDev = -1; } - // These were reset above - // didScheduleRenderPhaseUpdate = false; - // renderPhaseUpdates = null; - // numberOfReRenders = 0; + didScheduleRenderPhaseUpdate = false; invariant( !didRenderTooFewHooks, @@ -502,14 +494,29 @@ 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) { + // There were render phase updates. These are only valid for this render + // phase, which we are now aborting. Remove the updates from the queues so + // they do not persist to the next render. Do not remove updates from hooks + // that weren't processed. + // + // Only reset the updates from the queue if it has a clone. If it does + // not have a clone, that means it wasn't processed, and the updates were + // scheduled before we entered the render phase. + let hook: Hook | null = currentlyRenderingFiber.memoizedState; + while (hook !== null) { + const queue = hook.queue; + if (queue !== null) { + queue.pending = null; + } + hook = hook.next; + } + } renderExpirationTime = NoWork; currentlyRenderingFiber = (null: any); @@ -525,8 +532,6 @@ export function resetHooks(): void { } didScheduleRenderPhaseUpdate = false; - renderPhaseUpdates = null; - numberOfReRenders = 0; } function mountWorkInProgressHook(): Hook { @@ -662,49 +667,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. @@ -822,6 +784,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>] { @@ -852,6 +868,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, @@ -1165,6 +1187,23 @@ function updateDeferredValue( return prevValue; } +function rerenderDeferredValue( + value: T, + config: TimeoutConfig | void | null, +): T { + const [prevValue, setValue] = rerenderState(value); + updateEffect(() => { + const previousConfig = ReactCurrentBatchConfig.suspense; + ReactCurrentBatchConfig.suspense = config === undefined ? null : config; + try { + setValue(value); + } finally { + ReactCurrentBatchConfig.suspense = previousConfig; + } + }, [value, config]); + return prevValue; +} + function startTransition(setPending, config, callback) { const priorityLevel = getCurrentPriorityLevel(); runWithPriority( @@ -1210,17 +1249,22 @@ function updateTransition( return [start, isPending]; } +function rerenderTransition( + config: SuspenseConfig | void | null, +): [(() => void) => void, boolean] { + const [isPending, setPending] = rerenderState(false); + const start = updateCallback(startTransition.bind(null, setPending, config), [ + setPending, + config, + ]); + return [start, isPending]; +} + function dispatchAction( fiber: Fiber, 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( @@ -1231,6 +1275,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 || @@ -1240,64 +1316,9 @@ 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; + currentlyRenderingFiber.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) @@ -1402,11 +1423,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: createDeprecatedResponderListener, + useDeferredValue: rerenderDeferredValue, + useTransition: rerenderTransition, +}; + 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 = () => { @@ -1783,6 +1824,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 createDeprecatedResponderListener(responder, props); + }, + useDeferredValue(value: T, config: TimeoutConfig | void | null): T { + currentHookNameInDev = 'useDeferredValue'; + updateHookTypesDev(); + return rerenderDeferredValue(value, config); + }, + useTransition( + config: SuspenseConfig | void | null, + ): [(() => void) => void, boolean] { + currentHookNameInDev = 'useTransition'; + updateHookTypesDev(); + return rerenderTransition(config); + }, + }; + InvalidNestedHooksDispatcherOnMountInDEV = { readContext( context: ReactContext, @@ -2044,4 +2202,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 createDeprecatedResponderListener(responder, props); + }, + useDeferredValue(value: T, config: TimeoutConfig | void | null): T { + currentHookNameInDev = 'useDeferredValue'; + warnInvalidHookAccess(); + updateHookTypesDev(); + return rerenderDeferredValue(value, config); + }, + useTransition( + config: SuspenseConfig | void | null, + ): [(() => void) => void, boolean] { + currentHookNameInDev = 'useTransition'; + warnInvalidHookAccess(); + updateHookTypesDev(); + return rerenderTransition(config); + }, + }; } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index cb9de0ba4a88..a4664613ac22 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -141,7 +141,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) { @@ -2636,7 +2636,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 8ded101a09e1..ca4d3090215e 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -545,6 +545,132 @@ 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!']); + }); + + it('discards render phase updates if something suspends, but not other updates in the same component', async () => { + const thenable = {then() {}}; + function Foo({signal}) { + return ( + + + + ); + } + + let setLabel; + function Bar({signal: newSignal}) { + let [counter, setCounter] = useState(0); + + if (counter === 1) { + // 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; + } + + let [signal, setSignal] = useState(true); + + // Increment a counter every time the signal changes + if (signal !== newSignal) { + setCounter(c => c + 1); + setSignal(newSignal); + } + + let [label, _setLabel] = useState('A'); + setLabel = _setLabel; + + return ; + } + + const root = ReactNoop.createRoot(); + root.render(); + + expect(Scheduler).toFlushAndYield(['A:0']); + expect(root).toMatchRenderedOutput(); + + await ReactNoop.act(async () => { + root.render(); + setLabel('B'); + }); + expect(Scheduler).toHaveYielded(['Suspend!']); + expect(root).toMatchRenderedOutput(); + + // Rendering again should suspend again. + root.render(); + expect(Scheduler).toFlushAndYield(['Suspend!']); + + // Flip the signal back to "cancel" the update. However, the update to + // label should still proceed. It shouldn't have been dropped. + root.render(); + expect(Scheduler).toFlushAndYield(['B:0']); + expect(root).toMatchRenderedOutput(); + }); + + // TODO: This should probably warn + it.experimental('calling startTransition inside render phase', async () => { + let startTransition; + function App() { + let [counter, setCounter] = useState(0); + let [_startTransition] = useTransition(); + startTransition = _startTransition; + + if (counter === 0) { + startTransition(() => { + setCounter(c => c + 1); + }); + } + + return ; + } + + const root = ReactNoop.createRoot(); + root.render(); + expect(Scheduler).toFlushAndYield([1]); + expect(root).toMatchRenderedOutput(); + }); }); describe('useReducer', () => {