Skip to content

Commit

Permalink
Warn about async infinite useEffect loop (#15180)
Browse files Browse the repository at this point in the history
* Warn about async infinite useEffect loop

* Make tests sync
  • Loading branch information
gaearon authored Mar 22, 2019
1 parent 8e9a013 commit 5c2b2c0
Show file tree
Hide file tree
Showing 2 changed files with 135 additions and 0 deletions.
94 changes: 94 additions & 0 deletions packages/react-dom/src/__tests__/ReactUpdates-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,15 @@
let React;
let ReactDOM;
let ReactTestUtils;
let Scheduler;

describe('ReactUpdates', () => {
beforeEach(() => {
jest.resetModules();
React = require('react');
ReactDOM = require('react-dom');
ReactTestUtils = require('react-dom/test-utils');
Scheduler = require('scheduler');
});

it('should batch state when updating state twice', () => {
Expand Down Expand Up @@ -1524,4 +1526,96 @@ describe('ReactUpdates', () => {
});
});
});

if (__DEV__) {
it('warns about a deferred infinite update loop with useEffect', () => {
function NonTerminating() {
const [step, setStep] = React.useState(0);
React.useEffect(() => {
setStep(x => x + 1);
Scheduler.yieldValue(step);
});
return step;
}

function App() {
return <NonTerminating />;
}

let error = null;
let stack = null;
let originalConsoleError = console.error;
console.error = (e, s) => {
error = e;
stack = s;
};
try {
const container = document.createElement('div');
ReactDOM.render(<App />, container);
while (error === null) {
Scheduler.unstable_flushNumberOfYields(1);
}
expect(error).toContain('Warning: Maximum update depth exceeded.');
expect(stack).toContain('in NonTerminating');
} finally {
console.error = originalConsoleError;
}
});

it('can have nested updates if they do not cross the limit', () => {
let _setStep;
const LIMIT = 50;

function Terminating() {
const [step, setStep] = React.useState(0);
_setStep = setStep;
React.useEffect(() => {
if (step < LIMIT) {
setStep(x => x + 1);
Scheduler.yieldValue(step);
}
});
return step;
}

const container = document.createElement('div');
ReactDOM.render(<Terminating />, container);

// Verify we can flush them asynchronously without warning
for (let i = 0; i < LIMIT * 2; i++) {
Scheduler.unstable_flushNumberOfYields(1);
}
expect(container.textContent).toBe('50');

// Verify restarting from 0 doesn't cross the limit
expect(() => {
_setStep(0);
}).toWarnDev(
'An update to Terminating inside a test was not wrapped in act',
);
expect(container.textContent).toBe('0');
for (let i = 0; i < LIMIT * 2; i++) {
Scheduler.unstable_flushNumberOfYields(1);
}
expect(container.textContent).toBe('50');
});

it('can have many updates inside useEffect without triggering a warning', () => {
function Terminating() {
const [step, setStep] = React.useState(0);
React.useEffect(() => {
for (let i = 0; i < 1000; i++) {
setStep(x => x + 1);
}
Scheduler.yieldValue('Done');
}, []);
return step;
}

const container = document.createElement('div');
ReactDOM.render(<Terminating />, container);
expect(Scheduler).toFlushAndYield(['Done']);
expect(container.textContent).toBe('1000');
});
}
});
41 changes: 41 additions & 0 deletions packages/react-reconciler/src/ReactFiberScheduler.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ import {
} from 'shared/ReactFeatureFlags';
import getComponentName from 'shared/getComponentName';
import invariant from 'shared/invariant';
import warning from 'shared/warning';
import warningWithoutStack from 'shared/warningWithoutStack';

import ReactFiberInstrumentation from './ReactFiberInstrumentation';
Expand Down Expand Up @@ -547,7 +548,9 @@ function commitPassiveEffects(root: FiberRoot, firstEffect: Fiber): void {
let didError = false;
let error;
if (__DEV__) {
isInPassiveEffectDEV = true;
invokeGuardedCallback(null, commitPassiveHookEffects, null, effect);
isInPassiveEffectDEV = false;
if (hasCaughtError()) {
didError = true;
error = clearCaughtError();
Expand Down Expand Up @@ -581,6 +584,14 @@ function commitPassiveEffects(root: FiberRoot, firstEffect: Fiber): void {
if (!isBatchingUpdates && !isRendering) {
performSyncWork();
}

if (__DEV__) {
if (rootWithPendingPassiveEffects === root) {
nestedPassiveEffectCountDEV++;
} else {
nestedPassiveEffectCountDEV = 0;
}
}
}

function isAlreadyFailedLegacyErrorBoundary(instance: mixed): boolean {
Expand Down Expand Up @@ -1897,6 +1908,21 @@ function scheduleWork(fiber: Fiber, expirationTime: ExpirationTime) {
'the number of nested updates to prevent infinite loops.',
);
}
if (__DEV__) {
if (
isInPassiveEffectDEV &&
nestedPassiveEffectCountDEV > NESTED_PASSIVE_UPDATE_LIMIT
) {
nestedPassiveEffectCountDEV = 0;
warning(
false,
'Maximum update depth exceeded. This can happen when a ' +
'component calls setState inside useEffect, but ' +
"useEffect either doesn't have a dependency array, or " +
'one of the dependencies changes on every render.',
);
}
}
}

function deferredUpdates<A>(fn: () => A): A {
Expand Down Expand Up @@ -1962,6 +1988,15 @@ const NESTED_UPDATE_LIMIT = 50;
let nestedUpdateCount: number = 0;
let lastCommittedRootDuringThisBatch: FiberRoot | null = null;

// Similar, but for useEffect infinite loops. These are DEV-only.
const NESTED_PASSIVE_UPDATE_LIMIT = 50;
let nestedPassiveEffectCountDEV;
let isInPassiveEffectDEV;
if (__DEV__) {
nestedPassiveEffectCountDEV = 0;
isInPassiveEffectDEV = false;
}

function recomputeCurrentRendererTime() {
const currentTimeMs = now() - originalStartTimeMs;
currentRendererTime = msToExpirationTime(currentTimeMs);
Expand Down Expand Up @@ -2343,6 +2378,12 @@ function finishRendering() {
nestedUpdateCount = 0;
lastCommittedRootDuringThisBatch = null;

if (__DEV__) {
if (rootWithPendingPassiveEffects === null) {
nestedPassiveEffectCountDEV = 0;
}
}

if (completedBatches !== null) {
const batches = completedBatches;
completedBatches = null;
Expand Down

0 comments on commit 5c2b2c0

Please sign in to comment.