From ace6501b41a2b69fd734cd8b0d7135f716d6286c Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 20 Oct 2021 12:44:04 -0400 Subject: [PATCH 1/4] 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. --- .../src/ReactFiberHooks.new.js | 19 +++++-------------- .../src/ReactFiberHooks.old.js | 19 +++++-------------- .../src/ReactFiberWorkLoop.new.js | 7 ++++--- .../src/ReactFiberWorkLoop.old.js | 7 ++++--- 4 files changed, 18 insertions(+), 34 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index d21704a1b6f..afe0c268e9d 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -83,7 +83,7 @@ import { requestUpdateLane, requestEventTime, warnIfNotCurrentlyActingEffectsInDEV, - warnIfNotCurrentlyActingUpdatesInDev, + warnIfNotCurrentlyActingUpdatesInDEV, markSkippedUpdateLanes, isInterleavedUpdate, } from './ReactFiberWorkLoop.new'; @@ -118,7 +118,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 +1678,7 @@ function mountEffect( deps: Array | void | null, ): void { if (__DEV__) { - if (isActEnvironment(currentlyRenderingFiber)) { - warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber); - } + warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber); } if ( __DEV__ && @@ -1709,9 +1706,7 @@ function updateEffect( deps: Array | void | null, ): void { if (__DEV__) { - if (isActEnvironment(currentlyRenderingFiber)) { - warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber); - } + warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber); } return updateEffectImpl(PassiveEffect, HookPassive, create, deps); } @@ -2198,9 +2193,7 @@ function dispatchReducerAction( enqueueUpdate(fiber, queue, update, lane); if (__DEV__) { - if (isActEnvironment(fiber)) { - warnIfNotCurrentlyActingUpdatesInDev(fiber); - } + warnIfNotCurrentlyActingUpdatesInDEV(fiber); } const eventTime = requestEventTime(); const root = scheduleUpdateOnFiber(fiber, lane, eventTime); @@ -2283,9 +2276,7 @@ function dispatchSetState( } } if (__DEV__) { - if (isActEnvironment(fiber)) { - warnIfNotCurrentlyActingUpdatesInDev(fiber); - } + warnIfNotCurrentlyActingUpdatesInDEV(fiber); } const eventTime = requestEventTime(); const root = scheduleUpdateOnFiber(fiber, lane, eventTime); diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js index cf6786e617d..cbc7dfc3b0f 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -83,7 +83,7 @@ import { requestUpdateLane, requestEventTime, warnIfNotCurrentlyActingEffectsInDEV, - warnIfNotCurrentlyActingUpdatesInDev, + warnIfNotCurrentlyActingUpdatesInDEV, markSkippedUpdateLanes, isInterleavedUpdate, } from './ReactFiberWorkLoop.old'; @@ -118,7 +118,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 +1678,7 @@ function mountEffect( deps: Array | void | null, ): void { if (__DEV__) { - if (isActEnvironment(currentlyRenderingFiber)) { - warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber); - } + warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber); } if ( __DEV__ && @@ -1709,9 +1706,7 @@ function updateEffect( deps: Array | void | null, ): void { if (__DEV__) { - if (isActEnvironment(currentlyRenderingFiber)) { - warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber); - } + warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber); } return updateEffectImpl(PassiveEffect, HookPassive, create, deps); } @@ -2198,9 +2193,7 @@ function dispatchReducerAction( enqueueUpdate(fiber, queue, update, lane); if (__DEV__) { - if (isActEnvironment(fiber)) { - warnIfNotCurrentlyActingUpdatesInDev(fiber); - } + warnIfNotCurrentlyActingUpdatesInDEV(fiber); } const eventTime = requestEventTime(); const root = scheduleUpdateOnFiber(fiber, lane, eventTime); @@ -2283,9 +2276,7 @@ function dispatchSetState( } } if (__DEV__) { - if (isActEnvironment(fiber)) { - warnIfNotCurrentlyActingUpdatesInDev(fiber); - } + warnIfNotCurrentlyActingUpdatesInDEV(fiber); } const eventTime = requestEventTime(); const root = scheduleUpdateOnFiber(fiber, lane, eventTime); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 930fc608f47..5556a36fae9 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -236,6 +236,7 @@ import { } from './ReactFiberDevToolsHook.new'; import {onCommitRoot as onCommitRootTestSelector} from './ReactTestSelectors'; import {releaseCache} from './ReactFiberCacheComponent.new'; +import {isActEnvironment} from './ReactFiberAct.new'; const ceil = Math.ceil; @@ -2855,6 +2856,7 @@ function shouldForceFlushFallbacksInDEV() { export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void { if (__DEV__) { if ( + isActEnvironment(fiber) && (fiber.mode & StrictLegacyMode) !== NoMode && ReactCurrentActQueue.current === null ) { @@ -2875,9 +2877,10 @@ export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void { } } -function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void { +export function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void { if (__DEV__) { if ( + isActEnvironment(fiber) && executionContext === NoContext && ReactCurrentActQueue.current === null ) { @@ -2907,5 +2910,3 @@ function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void { } } } - -export const warnIfNotCurrentlyActingUpdatesInDev = warnIfNotCurrentlyActingUpdatesInDEV; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 0b7f46c7990..534466596f5 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -236,6 +236,7 @@ import { } from './ReactFiberDevToolsHook.old'; import {onCommitRoot as onCommitRootTestSelector} from './ReactTestSelectors'; import {releaseCache} from './ReactFiberCacheComponent.old'; +import {isActEnvironment} from './ReactFiberAct.old'; const ceil = Math.ceil; @@ -2855,6 +2856,7 @@ function shouldForceFlushFallbacksInDEV() { export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void { if (__DEV__) { if ( + isActEnvironment(fiber) && (fiber.mode & StrictLegacyMode) !== NoMode && ReactCurrentActQueue.current === null ) { @@ -2875,9 +2877,10 @@ export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void { } } -function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void { +export function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void { if (__DEV__) { if ( + isActEnvironment(fiber) && executionContext === NoContext && ReactCurrentActQueue.current === null ) { @@ -2907,5 +2910,3 @@ function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void { } } } - -export const warnIfNotCurrentlyActingUpdatesInDev = warnIfNotCurrentlyActingUpdatesInDEV; From 72a8460dc5eebf9b6e049a57f3154c592b737006 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 20 Oct 2021 12:48:07 -0400 Subject: [PATCH 2/4] 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. --- .../react-reconciler/src/ReactFiberAct.new.js | 54 ++++++++++--------- .../react-reconciler/src/ReactFiberAct.old.js | 54 ++++++++++--------- .../src/ReactFiberWorkLoop.new.js | 17 ++++-- .../src/ReactFiberWorkLoop.old.js | 17 ++++-- 4 files changed, 86 insertions(+), 56 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberAct.new.js b/packages/react-reconciler/src/ReactFiberAct.new.js index 18055e7c738..27102b5e925 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 ddae518dcb6..bcb4ad707a1 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/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 5556a36fae9..8965ce10465 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -236,7 +236,10 @@ import { } from './ReactFiberDevToolsHook.new'; import {onCommitRoot as onCommitRootTestSelector} from './ReactTestSelectors'; import {releaseCache} from './ReactFiberCacheComponent.new'; -import {isActEnvironment} from './ReactFiberAct.new'; +import { + isLegacyActEnvironment, + isConcurrentActEnvironment, +} from './ReactFiberAct.new'; const ceil = Math.ceil; @@ -2855,8 +2858,12 @@ function shouldForceFlushFallbacksInDEV() { export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void { if (__DEV__) { + const isActEnvironment = + fiber.mode & ConcurrentMode + ? isConcurrentActEnvironment() + : isLegacyActEnvironment(fiber); if ( - isActEnvironment(fiber) && + isActEnvironment && (fiber.mode & StrictLegacyMode) !== NoMode && ReactCurrentActQueue.current === null ) { @@ -2879,8 +2886,12 @@ export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void { export function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void { if (__DEV__) { + const isActEnvironment = + fiber.mode & ConcurrentMode + ? isConcurrentActEnvironment() + : isLegacyActEnvironment(fiber); if ( - isActEnvironment(fiber) && + isActEnvironment && executionContext === NoContext && ReactCurrentActQueue.current === null ) { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 534466596f5..c9ee956d525 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -236,7 +236,10 @@ import { } from './ReactFiberDevToolsHook.old'; import {onCommitRoot as onCommitRootTestSelector} from './ReactTestSelectors'; import {releaseCache} from './ReactFiberCacheComponent.old'; -import {isActEnvironment} from './ReactFiberAct.old'; +import { + isLegacyActEnvironment, + isConcurrentActEnvironment, +} from './ReactFiberAct.old'; const ceil = Math.ceil; @@ -2855,8 +2858,12 @@ function shouldForceFlushFallbacksInDEV() { export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void { if (__DEV__) { + const isActEnvironment = + fiber.mode & ConcurrentMode + ? isConcurrentActEnvironment() + : isLegacyActEnvironment(fiber); if ( - isActEnvironment(fiber) && + isActEnvironment && (fiber.mode & StrictLegacyMode) !== NoMode && ReactCurrentActQueue.current === null ) { @@ -2879,8 +2886,12 @@ export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void { export function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void { if (__DEV__) { + const isActEnvironment = + fiber.mode & ConcurrentMode + ? isConcurrentActEnvironment() + : isLegacyActEnvironment(fiber); if ( - isActEnvironment(fiber) && + isActEnvironment && executionContext === NoContext && ReactCurrentActQueue.current === null ) { From 846c1b194d9528a1ff8a1b963e30969c587f08aa Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 20 Oct 2021 17:36:04 -0400 Subject: [PATCH 3/4] Expand act warning to include non-hook APIs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../ReactDevToolsHooksIntegration-test.js | 28 +-- .../__tests__/preprocessData-test.internal.js | 6 +- .../src/__tests__/ReactTestUtilsAct-test.js | 38 +++- .../src/ReactFiberHooks.new.js | 8 - .../src/ReactFiberHooks.old.js | 8 - .../src/ReactFiberWorkLoop.new.js | 41 +++- .../src/ReactFiberWorkLoop.old.js | 41 +++- .../__tests__/DebugTracing-test.internal.js | 186 ++++++++---------- .../src/__tests__/ReactActWarnings-test.js | 60 ++++++ .../ReactFiberHostContext-test.internal.js | 2 + .../src/__tests__/ReactIsomorphicAct-test.js | 6 + 11 files changed, 257 insertions(+), 167 deletions(-) diff --git a/packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js b/packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js index 3eea121d65b..a64a8d794ad 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 94ada274a77..6cfb13a39a5 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(() => { @@ -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( diff --git a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js index d0992dce53d..556772500ee 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/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index afe0c268e9d..266e98960cb 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'; @@ -2191,10 +2190,6 @@ function dispatchReducerAction( enqueueRenderPhaseUpdate(queue, update); } else { enqueueUpdate(fiber, queue, update, lane); - - if (__DEV__) { - warnIfNotCurrentlyActingUpdatesInDEV(fiber); - } const eventTime = requestEventTime(); const root = scheduleUpdateOnFiber(fiber, lane, eventTime); if (root !== null) { @@ -2275,9 +2270,6 @@ function dispatchSetState( } } } - if (__DEV__) { - 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 cbc7dfc3b0f..b2378cbf30d 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'; @@ -2191,10 +2190,6 @@ function dispatchReducerAction( enqueueRenderPhaseUpdate(queue, update); } else { enqueueUpdate(fiber, queue, update, lane); - - if (__DEV__) { - warnIfNotCurrentlyActingUpdatesInDEV(fiber); - } const eventTime = requestEventTime(); const root = scheduleUpdateOnFiber(fiber, lane, eventTime); if (root !== null) { @@ -2275,9 +2270,6 @@ function dispatchSetState( } } } - if (__DEV__) { - 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 8965ce10465..f98d65a37b9 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -497,6 +497,8 @@ export function scheduleUpdateOnFiber( } } + warnIfUpdatesNotWrappedWithActDEV(fiber); + if (enableProfilerTimer && enableProfilerNestedUpdateScheduledHook) { if ( (executionContext & CommitContext) !== NoContext && @@ -2884,17 +2886,36 @@ export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void { } } -export function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void { +function warnIfUpdatesNotWrappedWithActDEV(fiber: Fiber): void { if (__DEV__) { - const isActEnvironment = - fiber.mode & ConcurrentMode - ? isConcurrentActEnvironment() - : isLegacyActEnvironment(fiber); - if ( - isActEnvironment && - 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); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index c9ee956d525..d1d8f249e9f 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -497,6 +497,8 @@ export function scheduleUpdateOnFiber( } } + warnIfUpdatesNotWrappedWithActDEV(fiber); + if (enableProfilerTimer && enableProfilerNestedUpdateScheduledHook) { if ( (executionContext & CommitContext) !== NoContext && @@ -2884,17 +2886,36 @@ export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void { } } -export function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void { +function warnIfUpdatesNotWrappedWithActDEV(fiber: Fiber): void { if (__DEV__) { - const isActEnvironment = - fiber.mode & ConcurrentMode - ? isConcurrentActEnvironment() - : isLegacyActEnvironment(fiber); - if ( - isActEnvironment && - 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); diff --git a/packages/react-reconciler/src/__tests__/DebugTracing-test.internal.js b/packages/react-reconciler/src/__tests__/DebugTracing-test.internal.js index ee93b3f9929..1fc035b8530 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([ @@ -147,21 +145,17 @@ describe('DebugTracing', () => { throw fakeSuspensePromise; } - 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', @@ -186,34 +180,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 +215,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 +245,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 +273,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 +349,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 +365,7 @@ describe('DebugTracing', () => { ]); }); + // @gate build === 'development' // @gate experimental || www it('should not log anything outside of a unstable_DebugTracingMode subtree', () => { function ExampleThatCascades() { @@ -417,16 +385,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 058e01b1fe0..99ec72c9fc8 100644 --- a/packages/react-reconciler/src/__tests__/ReactActWarnings-test.js +++ b/packages/react-reconciler/src/__tests__/ReactActWarnings-test.js @@ -127,4 +127,64 @@ 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'); + }); + }); }); diff --git a/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.internal.js b/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.internal.js index 35c22e672ed..ee4bd306481 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 135757d24f9..78458bd2d5b 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'); From 1961c9982274aac9ad9418759d1bea9b9ec50f9a Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 20 Oct 2021 18:36:36 -0400 Subject: [PATCH 4/4] 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. --- .../__tests__/preprocessData-test.internal.js | 6 +- .../src/ReactFiberWorkLoop.new.js | 26 +++ .../src/ReactFiberWorkLoop.old.js | 26 +++ .../__tests__/DebugTracing-test.internal.js | 17 +- .../src/__tests__/ReactActWarnings-test.js | 177 +++++++++++++++++- 5 files changed, 243 insertions(+), 9 deletions(-) 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 6cfb13a39a5..758afc1e15f 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 @@ -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'); } @@ -1839,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."`, @@ -1897,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-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index f98d65a37b9..2c065d6ee17 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -2408,6 +2408,8 @@ export function pingSuspendedRoot( const eventTime = requestEventTime(); markRootPinged(root, pingedLanes, eventTime); + warnIfSuspenseResolutionNotWrappedWithActDEV(root); + if ( workInProgressRoot === root && isSubsetOfLanes(workInProgressRootRenderLanes, pingedLanes) @@ -2942,3 +2944,27 @@ function warnIfUpdatesNotWrappedWithActDEV(fiber: Fiber): void { } } } + +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 d1d8f249e9f..97b835d3a18 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -2408,6 +2408,8 @@ export function pingSuspendedRoot( const eventTime = requestEventTime(); markRootPinged(root, pingedLanes, eventTime); + warnIfSuspenseResolutionNotWrappedWithActDEV(root); + if ( workInProgressRoot === root && isSubsetOfLanes(workInProgressRootRenderLanes, pingedLanes) @@ -2942,3 +2944,27 @@ function warnIfUpdatesNotWrappedWithActDEV(fiber: Fiber): void { } } } + +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 1fc035b8530..8be5b0bb972 100644 --- a/packages/react-reconciler/src/__tests__/DebugTracing-test.internal.js +++ b/packages/react-reconciler/src/__tests__/DebugTracing-test.internal.js @@ -140,9 +140,20 @@ 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.act(() => @@ -164,7 +175,7 @@ describe('DebugTracing', () => { logs.splice(0); - await fakeSuspensePromise; + await ReactTestRenderer.act(async () => await resolveFakeSuspensePromise()); expect(logs).toEqual(['log: ⚛️ Example resolved']); }); diff --git a/packages/react-reconciler/src/__tests__/ReactActWarnings-test.js b/packages/react-reconciler/src/__tests__/ReactActWarnings-test.js index 99ec72c9fc8..324d1532733 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) { @@ -187,4 +290,72 @@ describe('act warnings', () => { 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}, + ); + }); + }); });