Skip to content

Commit

Permalink
Expand act warning to include non-hook APIs
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@acdlite
acdlite committed Oct 20, 2021
1 parent c7f04c8 commit c21e9c0
Show file tree
Hide file tree
Showing 11 changed files with 244 additions and 168 deletions.
27 changes: 15 additions & 12 deletions packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js
View file
Original file line number Diff line number Diff line change
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,7 @@ describe('React hooks DevTools integration', () => {
}
});

it('should support overriding suspense in concurrent mode', () => {
it('should support overriding suspense in concurrent mode', async () => {
if (__DEV__) {
// Lock the first render
setSuspenseHandler(() => true);
Expand All @@ -243,13 +244,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
6 changes: 4 additions & 2 deletions ...-devtools-scheduling-profiler/src/import-worker/__tests__/preprocessData-test.internal.js
View file
Original file line number Diff line number Diff line change
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 @@ -1347,6 +1347,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
28 changes: 18 additions & 10 deletions packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,18 +32,20 @@ describe('ReactTestUtils.act()', () => {
let concurrentRoot = null;
const renderConcurrent = (el, dom) => {
concurrentRoot = ReactDOM.createRoot(dom);
concurrentRoot.render(el);
act(() => concurrentRoot.render(el));
};

const unmountConcurrent = _dom => {
if (concurrentRoot !== null) {
concurrentRoot.unmount();
concurrentRoot = null;
}
act(() => {
if (concurrentRoot !== null) {
concurrentRoot.unmount();
concurrentRoot = null;
}
});
};

const rerenderConcurrent = el => {
concurrentRoot.render(el);
act(() => concurrentRoot.render(el));
};

runActTests(
Expand Down Expand Up @@ -100,20 +102,26 @@ describe('ReactTestUtils.act()', () => {

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
8 changes: 0 additions & 8 deletions packages/react-reconciler/src/ReactFiberHooks.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ import {
requestUpdateLane,
requestEventTime,
warnIfNotCurrentlyActingEffectsInDEV,
warnIfNotCurrentlyActingUpdatesInDEV,
markSkippedUpdateLanes,
isInterleavedUpdate,
} from './ReactFiberWorkLoop.new';
Expand Down Expand Up @@ -2185,10 +2184,6 @@ function dispatchReducerAction<S, A>(
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) {
Expand Down Expand Up @@ -2269,9 +2264,6 @@ function dispatchSetState<S, A>(
}
}
}
if (__DEV__) {
warnIfNotCurrentlyActingUpdatesInDEV(fiber);
}
const eventTime = requestEventTime();
const root = scheduleUpdateOnFiber(fiber, lane, eventTime);
if (root !== null) {
Expand Down
8 changes: 0 additions & 8 deletions packages/react-reconciler/src/ReactFiberHooks.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ import {
requestUpdateLane,
requestEventTime,
warnIfNotCurrentlyActingEffectsInDEV,
warnIfNotCurrentlyActingUpdatesInDEV,
markSkippedUpdateLanes,
isInterleavedUpdate,
} from './ReactFiberWorkLoop.old';
Expand Down Expand Up @@ -2185,10 +2184,6 @@ function dispatchReducerAction<S, A>(
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) {
Expand Down Expand Up @@ -2269,9 +2264,6 @@ function dispatchSetState<S, A>(
}
}
}
if (__DEV__) {
warnIfNotCurrentlyActingUpdatesInDEV(fiber);
}
const eventTime = requestEventTime();
const root = scheduleUpdateOnFiber(fiber, lane, eventTime);
if (root !== null) {
Expand Down
41 changes: 31 additions & 10 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,8 @@ export function scheduleUpdateOnFiber(
}
}

warnIfUpdatesNotWrappedWithActDEV(fiber);

if (enableProfilerTimer && enableProfilerNestedUpdateScheduledHook) {
if (
(executionContext & CommitContext) !== NoContext &&
Expand Down Expand Up @@ -2844,17 +2846,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);
Expand Down
41 changes: 31 additions & 10 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,8 @@ export function scheduleUpdateOnFiber(
}
}

warnIfUpdatesNotWrappedWithActDEV(fiber);

if (enableProfilerTimer && enableProfilerNestedUpdateScheduledHook) {
if (
(executionContext & CommitContext) !== NoContext &&
Expand Down Expand Up @@ -2844,17 +2846,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);
Expand Down
Loading

0 comments on commit c21e9c0

Please sign in to comment.