From d5b6b4b865ebf13a1eaf2342d623101056e5e197 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 21 Oct 2021 18:31:26 -0400 Subject: [PATCH] Expand act warning to cover all APIs that might schedule React work (#22607) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Move isActEnvironment check to function that warns I'm about to fork the behavior in legacy roots versus concurrent roots even further, so I'm lifting this up so I only have to fork once. * Lift `mode` check, too Similar to previous commit. I only want to check this once. Not for performance reasons, but so the logic is easier to follow. * Expand act warning to include non-hook APIs In a test environment, React warns if an update isn't wrapped with act — but only if the update originates from a hook API, like useState. We did it this way for backwards compatibility with tests that were written before the act API was introduced. Those tests didn't require act, anyway, because in a legacy root, all tasks are synchronous except for `useEffect`. However, in a concurrent root, nearly every task is asynchronous. Even tasks that are synchronous may spawn additional asynchronous work. So all updates need to be wrapped with act, regardless of whether they originate from a hook, a class, a root, or any other type of component. This commit expands the act warning to include any API that triggers an update. It does not currently account for renders that are caused by a Suspense promise resolving; those are modelled slightly differently from updates. I'll fix that in the next step. I also removed the check for whether an update is batched. It shouldn't matter, because even a batched update can spawn asynchronous work, which needs to be flushed by act. This change only affects concurrent roots. The behavior in legacy roots is the same. * Expand act warning to include Suspense resolutions For the same reason we warn when an update is not wrapped with act, we should warn if a Suspense promise resolution is not wrapped with act. Both "pings" and "retries". Legacy root behavior is unchanged. --- .../ReactDevToolsHooksIntegration-test.js | 28 ++- .../__tests__/preprocessData-test.internal.js | 12 +- .../src/__tests__/ReactTestUtilsAct-test.js | 38 ++- .../react-reconciler/src/ReactFiberAct.new.js | 54 ++-- .../react-reconciler/src/ReactFiberAct.old.js | 54 ++-- .../src/ReactFiberHooks.new.js | 21 +- .../src/ReactFiberHooks.old.js | 21 +- .../src/ReactFiberWorkLoop.new.js | 71 +++++- .../src/ReactFiberWorkLoop.old.js | 71 +++++- .../__tests__/DebugTracing-test.internal.js | 203 +++++++-------- .../src/__tests__/ReactActWarnings-test.js | 237 +++++++++++++++++- .../ReactFiberHostContext-test.internal.js | 2 + .../src/__tests__/ReactIsomorphicAct-test.js | 6 + 13 files changed, 578 insertions(+), 240 deletions(-) diff --git a/packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js b/packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js index 3eea121d65b36..a64a8d794ad94 100644 --- a/packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js +++ b/packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js @@ -20,6 +20,8 @@ describe('React hooks DevTools integration', () => { let scheduleUpdate; let setSuspenseHandler; + global.IS_REACT_ACT_ENVIRONMENT = true; + beforeEach(() => { global.__REACT_DEVTOOLS_GLOBAL_HOOK__ = { inject: injected => { @@ -64,7 +66,7 @@ describe('React hooks DevTools integration', () => { expect(stateHook.isStateEditable).toBe(true); if (__DEV__) { - overrideHookState(fiber, stateHook.id, [], 10); + act(() => overrideHookState(fiber, stateHook.id, [], 10)); expect(renderer.toJSON()).toEqual({ type: 'div', props: {}, @@ -116,7 +118,7 @@ describe('React hooks DevTools integration', () => { expect(reducerHook.isStateEditable).toBe(true); if (__DEV__) { - overrideHookState(fiber, reducerHook.id, ['foo'], 'def'); + act(() => overrideHookState(fiber, reducerHook.id, ['foo'], 'def')); expect(renderer.toJSON()).toEqual({ type: 'div', props: {}, @@ -164,13 +166,12 @@ describe('React hooks DevTools integration', () => { expect(stateHook.isStateEditable).toBe(true); if (__DEV__) { - overrideHookState(fiber, stateHook.id, ['count'], 10); + act(() => overrideHookState(fiber, stateHook.id, ['count'], 10)); expect(renderer.toJSON()).toEqual({ type: 'div', props: {}, children: ['count:', '10'], }); - act(() => setStateFn(state => ({count: state.count + 1}))); expect(renderer.toJSON()).toEqual({ type: 'div', @@ -233,7 +234,8 @@ describe('React hooks DevTools integration', () => { } }); - it('should support overriding suspense in concurrent mode', () => { + // @gate __DEV__ + it('should support overriding suspense in concurrent mode', async () => { if (__DEV__) { // Lock the first render setSuspenseHandler(() => true); @@ -243,13 +245,15 @@ describe('React hooks DevTools integration', () => { return 'Done'; } - const renderer = ReactTestRenderer.create( -
- - - -
, - {unstable_isConcurrent: true}, + const renderer = await act(() => + ReactTestRenderer.create( +
+ + + +
, + {unstable_isConcurrent: true}, + ), ); expect(Scheduler).toFlushAndYield([]); diff --git a/packages/react-devtools-scheduling-profiler/src/import-worker/__tests__/preprocessData-test.internal.js b/packages/react-devtools-scheduling-profiler/src/import-worker/__tests__/preprocessData-test.internal.js index 94ada274a770b..758afc1e15f2c 100644 --- a/packages/react-devtools-scheduling-profiler/src/import-worker/__tests__/preprocessData-test.internal.js +++ b/packages/react-devtools-scheduling-profiler/src/import-worker/__tests__/preprocessData-test.internal.js @@ -17,8 +17,6 @@ import { } from '../../constants'; import REACT_VERSION from 'shared/ReactVersion'; -global.IS_REACT_ACT_ENVIRONMENT = true; - describe('getLanesFromTransportDecimalBitmask', () => { it('should return array of lane numbers from bitmask string', () => { expect(getLanesFromTransportDecimalBitmask('1')).toEqual([0]); @@ -210,6 +208,8 @@ describe('preprocessData', () => { tid = 0; pid = 0; startTime = 0; + + global.IS_REACT_ACT_ENVIRONMENT = true; }); afterEach(() => { @@ -1251,7 +1251,7 @@ describe('preprocessData', () => { testMarks.push(...createUserTimingData(clearedMarks)); - const data = await preprocessData(testMarks); + const data = await act(() => preprocessData(testMarks)); expect(data.suspenseEvents).toHaveLength(1); expect(data.suspenseEvents[0].promiseName).toBe('Testing displayName'); } @@ -1367,6 +1367,8 @@ describe('preprocessData', () => { const root = ReactDOM.createRoot(document.createElement('div')); + // Temporarily turn off the act environment, since we're intentionally using Scheduler instead. + global.IS_REACT_ACT_ENVIRONMENT = false; React.startTransition(() => { // Start rendering an async update (but don't finish). root.render( @@ -1837,7 +1839,7 @@ describe('preprocessData', () => { testMarks.push(...createUserTimingData(clearedMarks)); - const data = await preprocessData(testMarks); + const data = await act(() => preprocessData(testMarks)); expect(data.suspenseEvents).toHaveLength(1); expect(data.suspenseEvents[0].warning).toMatchInlineSnapshot( `"A component suspended during an update which caused a fallback to be shown. Consider using the Transition API to avoid hiding components after they've been mounted."`, @@ -1895,7 +1897,7 @@ describe('preprocessData', () => { testMarks.push(...createUserTimingData(clearedMarks)); - const data = await preprocessData(testMarks); + const data = await act(() => preprocessData(testMarks)); expect(data.suspenseEvents).toHaveLength(1); expect(data.suspenseEvents[0].warning).toBe(null); } diff --git a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js index d0992dce53d41..556772500ee2f 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js @@ -32,18 +32,31 @@ describe('ReactTestUtils.act()', () => { let concurrentRoot = null; const renderConcurrent = (el, dom) => { concurrentRoot = ReactDOM.createRoot(dom); - concurrentRoot.render(el); + if (__DEV__) { + act(() => concurrentRoot.render(el)); + } else { + concurrentRoot.render(el); + } }; const unmountConcurrent = _dom => { - if (concurrentRoot !== null) { - concurrentRoot.unmount(); - concurrentRoot = null; + if (__DEV__) { + act(() => { + if (concurrentRoot !== null) { + concurrentRoot.unmount(); + concurrentRoot = null; + } + }); + } else { + if (concurrentRoot !== null) { + concurrentRoot.unmount(); + concurrentRoot = null; + } } }; const rerenderConcurrent = el => { - concurrentRoot.render(el); + act(() => concurrentRoot.render(el)); }; runActTests( @@ -98,22 +111,29 @@ describe('ReactTestUtils.act()', () => { ]); }); + // @gate __DEV__ it('does not warn in concurrent mode', () => { const root = ReactDOM.createRoot(document.createElement('div')); - root.render(); + act(() => root.render()); Scheduler.unstable_flushAll(); }); it('warns in concurrent mode if root is strict', () => { + // TODO: We don't need this error anymore in concurrent mode because + // effects can only be scheduled as the result of an update, and we now + // enforce all updates must be wrapped with act, not just hook updates. expect(() => { const root = ReactDOM.createRoot(document.createElement('div'), { unstable_strictMode: true, }); root.render(); - Scheduler.unstable_flushAll(); - }).toErrorDev([ + }).toErrorDev( + 'An update to Root inside a test was not wrapped in act(...)', + {withoutStack: true}, + ); + expect(() => Scheduler.unstable_flushAll()).toErrorDev( 'An update to App ran an effect, but was not wrapped in act(...)', - ]); + ); }); }); }); diff --git a/packages/react-reconciler/src/ReactFiberAct.new.js b/packages/react-reconciler/src/ReactFiberAct.new.js index 18055e7c738c7..27102b5e92531 100644 --- a/packages/react-reconciler/src/ReactFiberAct.new.js +++ b/packages/react-reconciler/src/ReactFiberAct.new.js @@ -12,11 +12,32 @@ import type {Fiber} from './ReactFiber.new'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import {warnsIfNotActing} from './ReactFiberHostConfig'; -import {ConcurrentMode} from './ReactTypeOfMode'; const {ReactCurrentActQueue} = ReactSharedInternals; -export function isActEnvironment(fiber: Fiber) { +export function isLegacyActEnvironment(fiber: Fiber) { + if (__DEV__) { + // Legacy mode. We preserve the behavior of React 17's act. It assumes an + // act environment whenever `jest` is defined, but you can still turn off + // spurious warnings by setting IS_REACT_ACT_ENVIRONMENT explicitly + // to false. + + const isReactActEnvironmentGlobal = + // $FlowExpectedError – Flow doesn't know about IS_REACT_ACT_ENVIRONMENT global + typeof IS_REACT_ACT_ENVIRONMENT !== 'undefined' + ? IS_REACT_ACT_ENVIRONMENT + : undefined; + + // $FlowExpectedError - Flow doesn't know about jest + const jestIsDefined = typeof jest !== 'undefined'; + return ( + warnsIfNotActing && jestIsDefined && isReactActEnvironmentGlobal !== false + ); + } + return false; +} + +export function isConcurrentActEnvironment() { if (__DEV__) { const isReactActEnvironmentGlobal = // $FlowExpectedError – Flow doesn't know about IS_REACT_ACT_ENVIRONMENT global @@ -24,31 +45,14 @@ export function isActEnvironment(fiber: Fiber) { ? IS_REACT_ACT_ENVIRONMENT : undefined; - if (fiber.mode & ConcurrentMode) { - if ( - !isReactActEnvironmentGlobal && - ReactCurrentActQueue.current !== null - ) { - // TODO: Include link to relevant documentation page. - console.error( - 'The current testing environment is not configured to support ' + - 'act(...)', - ); - } - return isReactActEnvironmentGlobal; - } else { - // Legacy mode. We preserve the behavior of React 17's act. It assumes an - // act environment whenever `jest` is defined, but you can still turn off - // spurious warnings by setting IS_REACT_ACT_ENVIRONMENT explicitly - // to false. - // $FlowExpectedError - Flow doesn't know about jest - const jestIsDefined = typeof jest !== 'undefined'; - return ( - warnsIfNotActing && - jestIsDefined && - isReactActEnvironmentGlobal !== false + if (!isReactActEnvironmentGlobal && ReactCurrentActQueue.current !== null) { + // TODO: Include link to relevant documentation page. + console.error( + 'The current testing environment is not configured to support ' + + 'act(...)', ); } + return isReactActEnvironmentGlobal; } return false; } diff --git a/packages/react-reconciler/src/ReactFiberAct.old.js b/packages/react-reconciler/src/ReactFiberAct.old.js index ddae518dcb631..bcb4ad707a1be 100644 --- a/packages/react-reconciler/src/ReactFiberAct.old.js +++ b/packages/react-reconciler/src/ReactFiberAct.old.js @@ -12,11 +12,32 @@ import type {Fiber} from './ReactFiber.old'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import {warnsIfNotActing} from './ReactFiberHostConfig'; -import {ConcurrentMode} from './ReactTypeOfMode'; const {ReactCurrentActQueue} = ReactSharedInternals; -export function isActEnvironment(fiber: Fiber) { +export function isLegacyActEnvironment(fiber: Fiber) { + if (__DEV__) { + // Legacy mode. We preserve the behavior of React 17's act. It assumes an + // act environment whenever `jest` is defined, but you can still turn off + // spurious warnings by setting IS_REACT_ACT_ENVIRONMENT explicitly + // to false. + + const isReactActEnvironmentGlobal = + // $FlowExpectedError – Flow doesn't know about IS_REACT_ACT_ENVIRONMENT global + typeof IS_REACT_ACT_ENVIRONMENT !== 'undefined' + ? IS_REACT_ACT_ENVIRONMENT + : undefined; + + // $FlowExpectedError - Flow doesn't know about jest + const jestIsDefined = typeof jest !== 'undefined'; + return ( + warnsIfNotActing && jestIsDefined && isReactActEnvironmentGlobal !== false + ); + } + return false; +} + +export function isConcurrentActEnvironment() { if (__DEV__) { const isReactActEnvironmentGlobal = // $FlowExpectedError – Flow doesn't know about IS_REACT_ACT_ENVIRONMENT global @@ -24,31 +45,14 @@ export function isActEnvironment(fiber: Fiber) { ? IS_REACT_ACT_ENVIRONMENT : undefined; - if (fiber.mode & ConcurrentMode) { - if ( - !isReactActEnvironmentGlobal && - ReactCurrentActQueue.current !== null - ) { - // TODO: Include link to relevant documentation page. - console.error( - 'The current testing environment is not configured to support ' + - 'act(...)', - ); - } - return isReactActEnvironmentGlobal; - } else { - // Legacy mode. We preserve the behavior of React 17's act. It assumes an - // act environment whenever `jest` is defined, but you can still turn off - // spurious warnings by setting IS_REACT_ACT_ENVIRONMENT explicitly - // to false. - // $FlowExpectedError - Flow doesn't know about jest - const jestIsDefined = typeof jest !== 'undefined'; - return ( - warnsIfNotActing && - jestIsDefined && - isReactActEnvironmentGlobal !== false + if (!isReactActEnvironmentGlobal && ReactCurrentActQueue.current !== null) { + // TODO: Include link to relevant documentation page. + console.error( + 'The current testing environment is not configured to support ' + + 'act(...)', ); } + return isReactActEnvironmentGlobal; } return false; } diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index d21704a1b6fea..266e98960cbb3 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -83,7 +83,6 @@ import { requestUpdateLane, requestEventTime, warnIfNotCurrentlyActingEffectsInDEV, - warnIfNotCurrentlyActingUpdatesInDev, markSkippedUpdateLanes, isInterleavedUpdate, } from './ReactFiberWorkLoop.new'; @@ -118,7 +117,6 @@ import { } from './ReactUpdateQueue.new'; import {pushInterleavedQueue} from './ReactFiberInterleavedUpdates.new'; import {warnOnSubscriptionInsideStartTransition} from 'shared/ReactFeatureFlags'; -import {isActEnvironment} from './ReactFiberAct.new'; const {ReactCurrentDispatcher, ReactCurrentBatchConfig} = ReactSharedInternals; @@ -1679,9 +1677,7 @@ function mountEffect( deps: Array | void | null, ): void { if (__DEV__) { - if (isActEnvironment(currentlyRenderingFiber)) { - warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber); - } + warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber); } if ( __DEV__ && @@ -1709,9 +1705,7 @@ function updateEffect( deps: Array | void | null, ): void { if (__DEV__) { - if (isActEnvironment(currentlyRenderingFiber)) { - warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber); - } + warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber); } return updateEffectImpl(PassiveEffect, HookPassive, create, deps); } @@ -2196,12 +2190,6 @@ function dispatchReducerAction( enqueueRenderPhaseUpdate(queue, update); } else { enqueueUpdate(fiber, queue, update, lane); - - if (__DEV__) { - if (isActEnvironment(fiber)) { - warnIfNotCurrentlyActingUpdatesInDev(fiber); - } - } const eventTime = requestEventTime(); const root = scheduleUpdateOnFiber(fiber, lane, eventTime); if (root !== null) { @@ -2282,11 +2270,6 @@ function dispatchSetState( } } } - if (__DEV__) { - if (isActEnvironment(fiber)) { - warnIfNotCurrentlyActingUpdatesInDev(fiber); - } - } const eventTime = requestEventTime(); const root = scheduleUpdateOnFiber(fiber, lane, eventTime); if (root !== null) { diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js index cf6786e617daf..b2378cbf30d65 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -83,7 +83,6 @@ import { requestUpdateLane, requestEventTime, warnIfNotCurrentlyActingEffectsInDEV, - warnIfNotCurrentlyActingUpdatesInDev, markSkippedUpdateLanes, isInterleavedUpdate, } from './ReactFiberWorkLoop.old'; @@ -118,7 +117,6 @@ import { } from './ReactUpdateQueue.old'; import {pushInterleavedQueue} from './ReactFiberInterleavedUpdates.old'; import {warnOnSubscriptionInsideStartTransition} from 'shared/ReactFeatureFlags'; -import {isActEnvironment} from './ReactFiberAct.old'; const {ReactCurrentDispatcher, ReactCurrentBatchConfig} = ReactSharedInternals; @@ -1679,9 +1677,7 @@ function mountEffect( deps: Array | void | null, ): void { if (__DEV__) { - if (isActEnvironment(currentlyRenderingFiber)) { - warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber); - } + warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber); } if ( __DEV__ && @@ -1709,9 +1705,7 @@ function updateEffect( deps: Array | void | null, ): void { if (__DEV__) { - if (isActEnvironment(currentlyRenderingFiber)) { - warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber); - } + warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber); } return updateEffectImpl(PassiveEffect, HookPassive, create, deps); } @@ -2196,12 +2190,6 @@ function dispatchReducerAction( enqueueRenderPhaseUpdate(queue, update); } else { enqueueUpdate(fiber, queue, update, lane); - - if (__DEV__) { - if (isActEnvironment(fiber)) { - warnIfNotCurrentlyActingUpdatesInDev(fiber); - } - } const eventTime = requestEventTime(); const root = scheduleUpdateOnFiber(fiber, lane, eventTime); if (root !== null) { @@ -2282,11 +2270,6 @@ function dispatchSetState( } } } - if (__DEV__) { - if (isActEnvironment(fiber)) { - warnIfNotCurrentlyActingUpdatesInDev(fiber); - } - } const eventTime = requestEventTime(); const root = scheduleUpdateOnFiber(fiber, lane, eventTime); if (root !== null) { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 930fc608f4724..2c065d6ee17ab 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -236,6 +236,10 @@ import { } from './ReactFiberDevToolsHook.new'; import {onCommitRoot as onCommitRootTestSelector} from './ReactTestSelectors'; import {releaseCache} from './ReactFiberCacheComponent.new'; +import { + isLegacyActEnvironment, + isConcurrentActEnvironment, +} from './ReactFiberAct.new'; const ceil = Math.ceil; @@ -493,6 +497,8 @@ export function scheduleUpdateOnFiber( } } + warnIfUpdatesNotWrappedWithActDEV(fiber); + if (enableProfilerTimer && enableProfilerNestedUpdateScheduledHook) { if ( (executionContext & CommitContext) !== NoContext && @@ -2402,6 +2408,8 @@ export function pingSuspendedRoot( const eventTime = requestEventTime(); markRootPinged(root, pingedLanes, eventTime); + warnIfSuspenseResolutionNotWrappedWithActDEV(root); + if ( workInProgressRoot === root && isSubsetOfLanes(workInProgressRootRenderLanes, pingedLanes) @@ -2854,7 +2862,12 @@ function shouldForceFlushFallbacksInDEV() { export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void { if (__DEV__) { + const isActEnvironment = + fiber.mode & ConcurrentMode + ? isConcurrentActEnvironment() + : isLegacyActEnvironment(fiber); if ( + isActEnvironment && (fiber.mode & StrictLegacyMode) !== NoMode && ReactCurrentActQueue.current === null ) { @@ -2875,12 +2888,36 @@ export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void { } } -function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void { +function warnIfUpdatesNotWrappedWithActDEV(fiber: Fiber): void { if (__DEV__) { - if ( - executionContext === NoContext && - ReactCurrentActQueue.current === null - ) { + if (fiber.mode & ConcurrentMode) { + if (!isConcurrentActEnvironment()) { + // Not in an act environment. No need to warn. + return; + } + } else { + // Legacy mode has additional cases where we suppress a warning. + if (!isLegacyActEnvironment(fiber)) { + // Not in an act environment. No need to warn. + return; + } + if (executionContext !== NoContext) { + // Legacy mode doesn't warn if the update is batched, i.e. + // batchedUpdates or flushSync. + return; + } + if ( + fiber.tag !== FunctionComponent && + fiber.tag !== ForwardRef && + fiber.tag !== SimpleMemoComponent + ) { + // For backwards compatibility with pre-hooks code, legacy mode only + // warns for updates that originate from a hook. + return; + } + } + + if (ReactCurrentActQueue.current === null) { const previousFiber = ReactCurrentFiberCurrent; try { setCurrentDebugFiberInDEV(fiber); @@ -2908,4 +2945,26 @@ function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void { } } -export const warnIfNotCurrentlyActingUpdatesInDev = warnIfNotCurrentlyActingUpdatesInDEV; +function warnIfSuspenseResolutionNotWrappedWithActDEV(root: FiberRoot): void { + if (__DEV__) { + if ( + root.tag !== LegacyRoot && + isConcurrentActEnvironment() && + ReactCurrentActQueue.current === null + ) { + console.error( + 'A suspended resource finished loading inside a test, but the event ' + + 'was not wrapped in act(...).\n\n' + + 'When testing, code that resolves suspended data should be wrapped ' + + 'into act(...):\n\n' + + 'act(() => {\n' + + ' /* finish loading suspended data */\n' + + '});\n' + + '/* assert on the output */\n\n' + + "This ensures that you're testing the behavior the user would see " + + 'in the browser.' + + ' Learn more at https://reactjs.org/link/wrap-tests-with-act', + ); + } + } +} diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 0b7f46c799016..97b835d3a18e4 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -236,6 +236,10 @@ import { } from './ReactFiberDevToolsHook.old'; import {onCommitRoot as onCommitRootTestSelector} from './ReactTestSelectors'; import {releaseCache} from './ReactFiberCacheComponent.old'; +import { + isLegacyActEnvironment, + isConcurrentActEnvironment, +} from './ReactFiberAct.old'; const ceil = Math.ceil; @@ -493,6 +497,8 @@ export function scheduleUpdateOnFiber( } } + warnIfUpdatesNotWrappedWithActDEV(fiber); + if (enableProfilerTimer && enableProfilerNestedUpdateScheduledHook) { if ( (executionContext & CommitContext) !== NoContext && @@ -2402,6 +2408,8 @@ export function pingSuspendedRoot( const eventTime = requestEventTime(); markRootPinged(root, pingedLanes, eventTime); + warnIfSuspenseResolutionNotWrappedWithActDEV(root); + if ( workInProgressRoot === root && isSubsetOfLanes(workInProgressRootRenderLanes, pingedLanes) @@ -2854,7 +2862,12 @@ function shouldForceFlushFallbacksInDEV() { export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void { if (__DEV__) { + const isActEnvironment = + fiber.mode & ConcurrentMode + ? isConcurrentActEnvironment() + : isLegacyActEnvironment(fiber); if ( + isActEnvironment && (fiber.mode & StrictLegacyMode) !== NoMode && ReactCurrentActQueue.current === null ) { @@ -2875,12 +2888,36 @@ export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void { } } -function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void { +function warnIfUpdatesNotWrappedWithActDEV(fiber: Fiber): void { if (__DEV__) { - if ( - executionContext === NoContext && - ReactCurrentActQueue.current === null - ) { + if (fiber.mode & ConcurrentMode) { + if (!isConcurrentActEnvironment()) { + // Not in an act environment. No need to warn. + return; + } + } else { + // Legacy mode has additional cases where we suppress a warning. + if (!isLegacyActEnvironment(fiber)) { + // Not in an act environment. No need to warn. + return; + } + if (executionContext !== NoContext) { + // Legacy mode doesn't warn if the update is batched, i.e. + // batchedUpdates or flushSync. + return; + } + if ( + fiber.tag !== FunctionComponent && + fiber.tag !== ForwardRef && + fiber.tag !== SimpleMemoComponent + ) { + // For backwards compatibility with pre-hooks code, legacy mode only + // warns for updates that originate from a hook. + return; + } + } + + if (ReactCurrentActQueue.current === null) { const previousFiber = ReactCurrentFiberCurrent; try { setCurrentDebugFiberInDEV(fiber); @@ -2908,4 +2945,26 @@ function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void { } } -export const warnIfNotCurrentlyActingUpdatesInDev = warnIfNotCurrentlyActingUpdatesInDEV; +function warnIfSuspenseResolutionNotWrappedWithActDEV(root: FiberRoot): void { + if (__DEV__) { + if ( + root.tag !== LegacyRoot && + isConcurrentActEnvironment() && + ReactCurrentActQueue.current === null + ) { + console.error( + 'A suspended resource finished loading inside a test, but the event ' + + 'was not wrapped in act(...).\n\n' + + 'When testing, code that resolves suspended data should be wrapped ' + + 'into act(...):\n\n' + + 'act(() => {\n' + + ' /* finish loading suspended data */\n' + + '});\n' + + '/* assert on the output */\n\n' + + "This ensures that you're testing the behavior the user would see " + + 'in the browser.' + + ' Learn more at https://reactjs.org/link/wrap-tests-with-act', + ); + } + } +} diff --git a/packages/react-reconciler/src/__tests__/DebugTracing-test.internal.js b/packages/react-reconciler/src/__tests__/DebugTracing-test.internal.js index ee93b3f992994..8be5b0bb9720e 100644 --- a/packages/react-reconciler/src/__tests__/DebugTracing-test.internal.js +++ b/packages/react-reconciler/src/__tests__/DebugTracing-test.internal.js @@ -56,21 +56,17 @@ describe('DebugTracing', () => { expect(logs).toEqual([]); }); + // @gate build === 'development' // @gate experimental || www it('should not log anything for concurrent render without suspends or state updates', () => { - ReactTestRenderer.create( - -
- , - {unstable_isConcurrent: true}, + ReactTestRenderer.act(() => + ReactTestRenderer.create( + +
+ , + {unstable_isConcurrent: true}, + ), ); - - expect(logs).toEqual([]); - - logs.splice(0); - - expect(Scheduler).toFlushUntilNextPaint([]); - expect(logs).toEqual([]); }); @@ -81,12 +77,14 @@ describe('DebugTracing', () => { throw fakeSuspensePromise; } - ReactTestRenderer.create( - - - - - , + ReactTestRenderer.act(() => + ReactTestRenderer.create( + + + + + , + ), ); expect(logs).toEqual([ @@ -142,26 +140,33 @@ describe('DebugTracing', () => { // @gate experimental && build === 'development' && enableDebugTracing it('should log concurrent render with suspense', async () => { - const fakeSuspensePromise = Promise.resolve(true); + let isResolved = false; + let resolveFakeSuspensePromise; + const fakeSuspensePromise = new Promise(resolve => { + resolveFakeSuspensePromise = () => { + resolve(); + isResolved = true; + }; + }); + function Example() { - throw fakeSuspensePromise; + if (!isResolved) { + throw fakeSuspensePromise; + } + return null; } - ReactTestRenderer.create( - - - - - , - {unstable_isConcurrent: true}, + ReactTestRenderer.act(() => + ReactTestRenderer.create( + + + + + , + {unstable_isConcurrent: true}, + ), ); - expect(logs).toEqual([]); - - logs.splice(0); - - expect(Scheduler).toFlushUntilNextPaint([]); - expect(logs).toEqual([ `group: ⚛️ render (${DEFAULT_LANE_STRING})`, 'log: ⚛️ Example suspended', @@ -170,7 +175,7 @@ describe('DebugTracing', () => { logs.splice(0); - await fakeSuspensePromise; + await ReactTestRenderer.act(async () => await resolveFakeSuspensePromise()); expect(logs).toEqual(['log: ⚛️ Example resolved']); }); @@ -186,34 +191,23 @@ describe('DebugTracing', () => { return children; } - ReactTestRenderer.create( - - - - - - - , - {unstable_isConcurrent: true}, + ReactTestRenderer.act(() => + ReactTestRenderer.create( + + + + + + + , + {unstable_isConcurrent: true}, + ), ); - expect(logs).toEqual([]); - - logs.splice(0); - - expect(Scheduler).toFlushUntilNextPaint([]); - expect(logs).toEqual([ `group: ⚛️ render (${DEFAULT_LANE_STRING})`, 'log: ', `groupEnd: ⚛️ render (${DEFAULT_LANE_STRING})`, - ]); - - logs.splice(0); - - expect(Scheduler).toFlushUntilNextPaint([]); - - expect(logs).toEqual([ `group: ⚛️ render (${RETRY_LANE_STRING})`, 'log: ', `groupEnd: ⚛️ render (${RETRY_LANE_STRING})`, @@ -232,19 +226,15 @@ describe('DebugTracing', () => { } } - ReactTestRenderer.create( - - - , - {unstable_isConcurrent: true}, + ReactTestRenderer.act(() => + ReactTestRenderer.create( + + + , + {unstable_isConcurrent: true}, + ), ); - expect(logs).toEqual([]); - - logs.splice(0); - - expect(Scheduler).toFlushUntilNextPaint([]); - expect(logs).toEqual([ `group: ⚛️ commit (${DEFAULT_LANE_STRING})`, `group: ⚛️ layout effects (${DEFAULT_LANE_STRING})`, @@ -266,19 +256,15 @@ describe('DebugTracing', () => { } } - ReactTestRenderer.create( - - - , - {unstable_isConcurrent: true}, - ); - - expect(logs).toEqual([]); - - logs.splice(0); - expect(() => { - expect(Scheduler).toFlushUntilNextPaint([]); + ReactTestRenderer.act(() => + ReactTestRenderer.create( + + + , + {unstable_isConcurrent: true}, + ), + ); }).toErrorDev('Cannot update during an existing state transition'); expect(logs).toEqual([ @@ -298,19 +284,15 @@ describe('DebugTracing', () => { return didMount; } - ReactTestRenderer.create( - - - , - {unstable_isConcurrent: true}, + ReactTestRenderer.act(() => + ReactTestRenderer.create( + + + , + {unstable_isConcurrent: true}, + ), ); - expect(logs).toEqual([]); - - logs.splice(0); - - expect(Scheduler).toFlushUntilNextPaint([]); - expect(logs).toEqual([ `group: ⚛️ commit (${DEFAULT_LANE_STRING})`, `group: ⚛️ layout effects (${DEFAULT_LANE_STRING})`, @@ -378,19 +360,15 @@ describe('DebugTracing', () => { return null; } - ReactTestRenderer.create( - - - , - {unstable_isConcurrent: true}, + ReactTestRenderer.act(() => + ReactTestRenderer.create( + + + , + {unstable_isConcurrent: true}, + ), ); - expect(logs).toEqual([]); - - logs.splice(0); - - expect(Scheduler).toFlushUntilNextPaint([]); - expect(logs).toEqual([ `group: ⚛️ render (${DEFAULT_LANE_STRING})`, 'log: Hello from user code', @@ -398,6 +376,7 @@ describe('DebugTracing', () => { ]); }); + // @gate build === 'development' // @gate experimental || www it('should not log anything outside of a unstable_DebugTracingMode subtree', () => { function ExampleThatCascades() { @@ -417,16 +396,18 @@ describe('DebugTracing', () => { return null; } - ReactTestRenderer.create( - - - - - - - - - , + ReactTestRenderer.act(() => + ReactTestRenderer.create( + + + + + + + + + , + ), ); expect(logs).toEqual([]); diff --git a/packages/react-reconciler/src/__tests__/ReactActWarnings-test.js b/packages/react-reconciler/src/__tests__/ReactActWarnings-test.js index 058e01b1fe060..324d153273361 100644 --- a/packages/react-reconciler/src/__tests__/ReactActWarnings-test.js +++ b/packages/react-reconciler/src/__tests__/ReactActWarnings-test.js @@ -12,6 +12,10 @@ let Scheduler; let ReactNoop; let useState; let act; +let Suspense; +let startTransition; +let getCacheForType; +let caches; // These tests are mostly concerned with concurrent roots. The legacy root // behavior is covered by other older test suites and is unchanged from @@ -24,11 +28,110 @@ describe('act warnings', () => { ReactNoop = require('react-noop-renderer'); act = React.unstable_act; useState = React.useState; + Suspense = React.Suspense; + startTransition = React.startTransition; + getCacheForType = React.unstable_getCacheForType; + caches = []; }); - function Text(props) { - Scheduler.unstable_yieldValue(props.text); - return props.text; + function createTextCache() { + const data = new Map(); + const version = caches.length + 1; + const cache = { + version, + data, + resolve(text) { + const record = data.get(text); + if (record === undefined) { + const newRecord = { + status: 'resolved', + value: text, + }; + data.set(text, newRecord); + } else if (record.status === 'pending') { + const thenable = record.value; + record.status = 'resolved'; + record.value = text; + thenable.pings.forEach(t => t()); + } + }, + reject(text, error) { + const record = data.get(text); + if (record === undefined) { + const newRecord = { + status: 'rejected', + value: error, + }; + data.set(text, newRecord); + } else if (record.status === 'pending') { + const thenable = record.value; + record.status = 'rejected'; + record.value = error; + thenable.pings.forEach(t => t()); + } + }, + }; + caches.push(cache); + return cache; + } + + function readText(text) { + const textCache = getCacheForType(createTextCache); + const record = textCache.data.get(text); + if (record !== undefined) { + switch (record.status) { + case 'pending': + Scheduler.unstable_yieldValue(`Suspend! [${text}]`); + throw record.value; + case 'rejected': + Scheduler.unstable_yieldValue(`Error! [${text}]`); + throw record.value; + case 'resolved': + return textCache.version; + } + } else { + Scheduler.unstable_yieldValue(`Suspend! [${text}]`); + + const thenable = { + pings: [], + then(resolve) { + if (newRecord.status === 'pending') { + thenable.pings.push(resolve); + } else { + Promise.resolve().then(() => resolve(newRecord.value)); + } + }, + }; + + const newRecord = { + status: 'pending', + value: thenable, + }; + textCache.data.set(text, newRecord); + + throw thenable; + } + } + + function Text({text}) { + Scheduler.unstable_yieldValue(text); + return text; + } + + function AsyncText({text}) { + readText(text); + Scheduler.unstable_yieldValue(text); + return text; + } + + function resolveText(text) { + if (caches.length === 0) { + throw Error('Cache does not exist.'); + } else { + // Resolve the most recently created cache. An older cache can by + // resolved with `caches[index].resolve(text)`. + caches[caches.length - 1].resolve(text); + } } function withActEnvironment(value, scope) { @@ -127,4 +230,132 @@ describe('act warnings', () => { expect(root).toMatchRenderedOutput('1'); }); }); + + test('warns if root update is not wrapped', () => { + withActEnvironment(true, () => { + const root = ReactNoop.createRoot(); + expect(() => root.render('Hi')).toErrorDev( + // TODO: Better error message that doesn't make it look like "Root" is + // the name of a custom component + 'An update to Root inside a test was not wrapped in act(...)', + {withoutStack: true}, + ); + }); + }); + + // @gate __DEV__ + test('warns if class update is not wrapped', () => { + let app; + class App extends React.Component { + state = {count: 0}; + render() { + app = this; + return ; + } + } + + withActEnvironment(true, () => { + const root = ReactNoop.createRoot(); + act(() => { + root.render(); + }); + expect(() => app.setState({count: 1})).toErrorDev( + 'An update to App inside a test was not wrapped in act(...)', + ); + }); + }); + + // @gate __DEV__ + test('warns even if update is synchronous', () => { + let setState; + function App() { + const [state, _setState] = useState(0); + setState = _setState; + return ; + } + + withActEnvironment(true, () => { + const root = ReactNoop.createRoot(); + act(() => root.render()); + expect(Scheduler).toHaveYielded([0]); + expect(root).toMatchRenderedOutput('0'); + + // Even though this update is synchronous, we should still fire a warning, + // because it could have spawned additional asynchronous work + expect(() => ReactNoop.flushSync(() => setState(1))).toErrorDev( + 'An update to App inside a test was not wrapped in act(...)', + ); + + expect(Scheduler).toHaveYielded([1]); + expect(root).toMatchRenderedOutput('1'); + }); + }); + + // @gate __DEV__ + // @gate enableCache + test('warns if Suspense retry is not wrapped', () => { + function App() { + return ( + }> + + + ); + } + + withActEnvironment(true, () => { + const root = ReactNoop.createRoot(); + act(() => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['Suspend! [Async]', 'Loading...']); + expect(root).toMatchRenderedOutput('Loading...'); + + // This is a retry, not a ping, because we already showed a fallback. + expect(() => + resolveText('Async'), + ).toErrorDev( + 'A suspended resource finished loading inside a test, but the event ' + + 'was not wrapped in act(...)', + {withoutStack: true}, + ); + }); + }); + + // @gate __DEV__ + // @gate enableCache + test('warns if Suspense ping is not wrapped', () => { + function App({showMore}) { + return ( + }> + {showMore ? : } + + ); + } + + withActEnvironment(true, () => { + const root = ReactNoop.createRoot(); + act(() => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['(empty)']); + expect(root).toMatchRenderedOutput('(empty)'); + + act(() => { + startTransition(() => { + root.render(); + }); + }); + expect(Scheduler).toHaveYielded(['Suspend! [Async]', 'Loading...']); + expect(root).toMatchRenderedOutput('(empty)'); + + // This is a ping, not a retry, because no fallback is showing. + expect(() => + resolveText('Async'), + ).toErrorDev( + 'A suspended resource finished loading inside a test, but the event ' + + 'was not wrapped in act(...)', + {withoutStack: true}, + ); + }); + }); }); diff --git a/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.internal.js b/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.internal.js index 35c22e672ed3e..ee4bd306481b0 100644 --- a/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.internal.js @@ -28,6 +28,8 @@ describe('ReactFiberHostContext', () => { .DefaultEventPriority; }); + global.IS_REACT_ACT_ENVIRONMENT = true; + // @gate __DEV__ it('works with null host context', async () => { let creates = 0; diff --git a/packages/react-reconciler/src/__tests__/ReactIsomorphicAct-test.js b/packages/react-reconciler/src/__tests__/ReactIsomorphicAct-test.js index 135757d24f9e1..78458bd2d5bfd 100644 --- a/packages/react-reconciler/src/__tests__/ReactIsomorphicAct-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIsomorphicAct-test.js @@ -23,12 +23,17 @@ describe('isomorphic act()', () => { act = React.unstable_act; }); + beforeEach(() => { + global.IS_REACT_ACT_ENVIRONMENT = true; + }); + // @gate __DEV__ test('bypasses queueMicrotask', async () => { const root = ReactNoop.createRoot(); // First test what happens without wrapping in act. This update would // normally be queued in a microtask. + global.IS_REACT_ACT_ENVIRONMENT = false; ReactNoop.unstable_runWithPriority(DiscreteEventPriority, () => { root.render('A'); }); @@ -40,6 +45,7 @@ describe('isomorphic act()', () => { // Now do the same thing but wrap the update with `act`. No // `await` necessary. + global.IS_REACT_ACT_ENVIRONMENT = true; act(() => { ReactNoop.unstable_runWithPriority(DiscreteEventPriority, () => { root.render('B');