Skip to content

Commit

Permalink
Remove warning for dangling passive effects (#22609)
Browse files Browse the repository at this point in the history
In legacy mode, a test can get into a situation where passive effects are
"dangling" — an update finished, and scheduled some passive effects,
but the effects don't flush.

This is why React warns if you don't wrap updates in act. The act API is
responsible for flushing passive effects. But there are some cases where
the act API (in legacy roots) intentionally doesn't warn, like updates
that originate from roots and classes. It's possible those updates will
render children that contain useEffect. Because of this, dangling
effects are still possible, and React doesn't warn about it.

So we implemented a second act warning for dangling effects.

However, in concurrent roots, we now enforce that all APIs that schedule
React work must be wrapped in act. There's no scenario where dangling
passive effects can happen that doesn't already trigger the warning for
updates. So the dangling effects warning is redundant.

The warning was never part of a public release. It was only enabled
in concurrent roots.

So we can delete it.
  • Loading branch information
acdlite authored Oct 21, 2021
1 parent d5b6b4b commit 3c4c1c4
Show file tree
Hide file tree
Showing 5 changed files with 2 additions and 113 deletions.
31 changes: 0 additions & 31 deletions packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,43 +98,12 @@ describe('ReactTestUtils.act()', () => {
}).toErrorDev([]);
});

it('warns in strict mode', () => {
expect(() => {
ReactDOM.render(
<React.StrictMode>
<App />
</React.StrictMode>,
document.createElement('div'),
);
}).toErrorDev([
'An update to App ran an effect, but was not wrapped in act(...)',
]);
});

// @gate __DEV__
it('does not warn in concurrent mode', () => {
const root = ReactDOM.createRoot(document.createElement('div'));
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 />);
}).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
7 changes: 0 additions & 7 deletions packages/react-reconciler/src/ReactFiberHooks.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ import {
scheduleUpdateOnFiber,
requestUpdateLane,
requestEventTime,
warnIfNotCurrentlyActingEffectsInDEV,
markSkippedUpdateLanes,
isInterleavedUpdate,
} from './ReactFiberWorkLoop.new';
Expand Down Expand Up @@ -1676,9 +1675,6 @@ function mountEffect(
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
if (__DEV__) {
warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber);
}
if (
__DEV__ &&
enableStrictEffects &&
Expand All @@ -1704,9 +1700,6 @@ function updateEffect(
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
if (__DEV__) {
warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber);
}
return updateEffectImpl(PassiveEffect, HookPassive, create, deps);
}

Expand Down
7 changes: 0 additions & 7 deletions packages/react-reconciler/src/ReactFiberHooks.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ import {
scheduleUpdateOnFiber,
requestUpdateLane,
requestEventTime,
warnIfNotCurrentlyActingEffectsInDEV,
markSkippedUpdateLanes,
isInterleavedUpdate,
} from './ReactFiberWorkLoop.old';
Expand Down Expand Up @@ -1676,9 +1675,6 @@ function mountEffect(
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
if (__DEV__) {
warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber);
}
if (
__DEV__ &&
enableStrictEffects &&
Expand All @@ -1704,9 +1700,6 @@ function updateEffect(
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
if (__DEV__) {
warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber);
}
return updateEffectImpl(PassiveEffect, HookPassive, create, deps);
}

Expand Down
35 changes: 1 addition & 34 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,12 +97,7 @@ import {
createWorkInProgress,
assignFiberPropertiesInDEV,
} from './ReactFiber.new';
import {
NoMode,
StrictLegacyMode,
ProfileMode,
ConcurrentMode,
} from './ReactTypeOfMode';
import {NoMode, ProfileMode, ConcurrentMode} from './ReactTypeOfMode';
import {
HostRoot,
IndeterminateComponent,
Expand Down Expand Up @@ -2860,34 +2855,6 @@ function shouldForceFlushFallbacksInDEV() {
return __DEV__ && ReactCurrentActQueue.current !== null;
}

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
) {
console.error(
'An update to %s ran an effect, but was not wrapped in act(...).\n\n' +
'When testing, code that causes React state updates should be ' +
'wrapped into act(...):\n\n' +
'act(() => {\n' +
' /* fire events that update state */\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',
getComponentNameFromFiber(fiber),
);
}
}
}

function warnIfUpdatesNotWrappedWithActDEV(fiber: Fiber): void {
if (__DEV__) {
if (fiber.mode & ConcurrentMode) {
Expand Down
35 changes: 1 addition & 34 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,12 +97,7 @@ import {
createWorkInProgress,
assignFiberPropertiesInDEV,
} from './ReactFiber.old';
import {
NoMode,
StrictLegacyMode,
ProfileMode,
ConcurrentMode,
} from './ReactTypeOfMode';
import {NoMode, ProfileMode, ConcurrentMode} from './ReactTypeOfMode';
import {
HostRoot,
IndeterminateComponent,
Expand Down Expand Up @@ -2860,34 +2855,6 @@ function shouldForceFlushFallbacksInDEV() {
return __DEV__ && ReactCurrentActQueue.current !== null;
}

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
) {
console.error(
'An update to %s ran an effect, but was not wrapped in act(...).\n\n' +
'When testing, code that causes React state updates should be ' +
'wrapped into act(...):\n\n' +
'act(() => {\n' +
' /* fire events that update state */\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',
getComponentNameFromFiber(fiber),
);
}
}
}

function warnIfUpdatesNotWrappedWithActDEV(fiber: Fiber): void {
if (__DEV__) {
if (fiber.mode & ConcurrentMode) {
Expand Down

0 comments on commit 3c4c1c4

Please sign in to comment.