Skip to content

Commit

Permalink
Expand act warning to cover all APIs that might schedule React work (#…
Browse files Browse the repository at this point in the history 
…22607)

* 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.
  • Loading branch information
@acdlite
acdlite committed Oct 21, 2021
1 parent fa9bea0 commit d5b6b4b
Show file tree
Hide file tree
Showing 13 changed files with 578 additions and 240 deletions.
28 changes: 16 additions & 12 deletions packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js
View file
Expand Up @@ -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 => {
Expand Down Expand Up @@ -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: {},
Expand Down Expand Up @@ -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: {},
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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);
Expand All @@ -243,13 +245,15 @@ describe('React hooks DevTools integration', () => {
return 'Done';
}

const renderer = ReactTestRenderer.create(
<div>
<React.Suspense fallback={'Loading'}>
<MyComponent />
</React.Suspense>
</div>,
{unstable_isConcurrent: true},
const renderer = await act(() =>
ReactTestRenderer.create(
<div>
<React.Suspense fallback={'Loading'}>
<MyComponent />
</React.Suspense>
</div>,
{unstable_isConcurrent: true},
),
);

expect(Scheduler).toFlushAndYield([]);
Expand Down
12 changes: 7 additions & 5 deletions ...-devtools-scheduling-profiler/src/import-worker/__tests__/preprocessData-test.internal.js
View file
Expand Up @@ -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]);
Expand Down Expand Up @@ -210,6 +208,8 @@ describe('preprocessData', () => {
tid = 0;
pid = 0;
startTime = 0;

global.IS_REACT_ACT_ENVIRONMENT = true;
});

afterEach(() => {
Expand Down Expand Up @@ -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');
}
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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."`,
Expand Down Expand Up @@ -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);
}
Expand Down
38 changes: 29 additions & 9 deletions packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js
View file
Expand Up @@ -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(
Expand Down Expand Up @@ -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(<App />);
act(() => root.render(<App />));
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(<App />);
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(...)',
]);
);
});
});
});
Expand Down
54 changes: 29 additions & 25 deletions packages/react-reconciler/src/ReactFiberAct.new.js
View file
Expand Up @@ -12,43 +12,47 @@ 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
typeof IS_REACT_ACT_ENVIRONMENT !== 'undefined'
? 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;
}
54 changes: 29 additions & 25 deletions packages/react-reconciler/src/ReactFiberAct.old.js
View file
Expand Up @@ -12,43 +12,47 @@ 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
typeof IS_REACT_ACT_ENVIRONMENT !== 'undefined'
? 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;
}

0 comments on commit d5b6b4b

Please sign in to comment.