Skip to content

Commit

Permalink
Warn about setState directly in dep-less useEffect (#15184)
Browse files Browse the repository at this point in the history
  • Loading branch information
gaearon authored Mar 22, 2019
1 parent 78f2775 commit b1cccd1
Show file tree
Hide file tree
Showing 2 changed files with 200 additions and 37 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -992,6 +992,18 @@ const tests = {
}
`,
},
{
code: `
function Hello() {
const [state, setState] = useState(0);
useEffect(() => {
const handleResize = () => setState(window.innerWidth);
window.addEventListener('resize', handleResize);
return () => window.removeEventListener('resize', handleResize);
});
}
`,
},
],
invalid: [
{
Expand Down Expand Up @@ -4462,6 +4474,102 @@ const tests = {
`Either exclude it or remove the dependency array.`,
],
},
{
code: `
function Hello() {
const [state, setState] = useState(0);
useEffect(() => {
setState({});
});
}
`,
output: `
function Hello() {
const [state, setState] = useState(0);
useEffect(() => {
setState({});
}, []);
}
`,
errors: [
`React Hook useEffect contains a call to 'setState'. ` +
`Without a list of dependencies, this can lead to an infinite chain of updates. ` +
`To fix this, pass [] as a second argument to the useEffect Hook.`,
],
},
{
code: `
function Hello() {
const [data, setData] = useState(0);
useEffect(() => {
fetchData.then(setData);
});
}
`,
output: `
function Hello() {
const [data, setData] = useState(0);
useEffect(() => {
fetchData.then(setData);
}, []);
}
`,
errors: [
`React Hook useEffect contains a call to 'setData'. ` +
`Without a list of dependencies, this can lead to an infinite chain of updates. ` +
`To fix this, pass [] as a second argument to the useEffect Hook.`,
],
},
{
code: `
function Hello({ country }) {
const [data, setData] = useState(0);
useEffect(() => {
fetchData(country).then(setData);
});
}
`,
output: `
function Hello({ country }) {
const [data, setData] = useState(0);
useEffect(() => {
fetchData(country).then(setData);
}, [country]);
}
`,
errors: [
`React Hook useEffect contains a call to 'setData'. ` +
`Without a list of dependencies, this can lead to an infinite chain of updates. ` +
`To fix this, pass [country] as a second argument to the useEffect Hook.`,
],
},
{
code: `
function Hello({ prop1, prop2 }) {
const [state, setState] = useState(0);
useEffect(() => {
if (prop1) {
setState(prop2);
}
});
}
`,
output: `
function Hello({ prop1, prop2 }) {
const [state, setState] = useState(0);
useEffect(() => {
if (prop1) {
setState(prop2);
}
}, [prop1, prop2]);
}
`,
errors: [
`React Hook useEffect contains a call to 'setState'. ` +
`Without a list of dependencies, this can lead to an infinite chain of updates. ` +
`To fix this, pass [prop1, prop2] as a second argument to the useEffect Hook.`,
],
},
{
code: `
function Thing() {
Expand Down
129 changes: 92 additions & 37 deletions packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
Original file line number Diff line number Diff line change
Expand Up @@ -468,9 +468,101 @@ export default {
},
);

// Warn about assigning to variables in the outer scope.
// Those are usually bugs.
let staleAssignments = new Set();
function reportStaleAssignment(writeExpr, key) {
if (staleAssignments.has(key)) {
return;
}
staleAssignments.add(key);
context.report({
node: writeExpr,
message:
`Assignments to the '${key}' variable from inside React Hook ` +
`${context.getSource(reactiveHook)} will be lost after each ` +
`render. To preserve the value over time, store it in a useRef ` +
`Hook and keep the mutable value in the '.current' property. ` +
`Otherwise, you can move this variable directly inside ` +
`${context.getSource(reactiveHook)}.`,
});
}

// Remember which deps are optional and report bad usage first.
const optionalDependencies = new Set();
dependencies.forEach(({isStatic, references}, key) => {
if (isStatic) {
optionalDependencies.add(key);
}
references.forEach(reference => {
if (reference.writeExpr) {
reportStaleAssignment(reference.writeExpr, key);
}
});
});

if (staleAssignments.size > 0) {
// The intent isn't clear so we'll wait until you fix those first.
return;
}

if (!declaredDependenciesNode) {
// Check if there are any top-level setState() calls.
// Those tend to lead to infinite loops.
let setStateInsideEffectWithoutDeps = null;
dependencies.forEach(({isStatic, references}, key) => {
if (setStateInsideEffectWithoutDeps) {
return;
}
references.forEach(reference => {
if (setStateInsideEffectWithoutDeps) {
return;
}

const id = reference.identifier;
const isSetState = setStateCallSites.has(id);
if (!isSetState) {
return;
}

let fnScope = reference.from;
while (fnScope.type !== 'function') {
fnScope = fnScope.upper;
}
const isDirectlyInsideEffect = fnScope.block === node;
if (isDirectlyInsideEffect) {
// TODO: we could potentially ignore early returns.
setStateInsideEffectWithoutDeps = key;
}
});
});
if (setStateInsideEffectWithoutDeps) {
let {suggestedDependencies} = collectRecommendations({
dependencies,
declaredDependencies: [],
optionalDependencies,
externalDependencies: new Set(),
isEffect: true,
});
context.report({
node: node.parent.callee,
message:
`React Hook ${reactiveHookName} contains a call to '${setStateInsideEffectWithoutDeps}'. ` +
`Without a list of dependencies, this can lead to an infinite chain of updates. ` +
`To fix this, pass [` +
suggestedDependencies.join(', ') +
`] as a second argument to the ${reactiveHookName} Hook.`,
fix(fixer) {
return fixer.insertTextAfter(
node,
`, [${suggestedDependencies.join(', ')}]`,
);
},
});
}
return;
}

const declaredDependencies = [];
const externalDependencies = new Set();
if (declaredDependenciesNode.type !== 'ArrayExpression') {
Expand Down Expand Up @@ -569,43 +661,6 @@ export default {
});
}

// Warn about assigning to variables in the outer scope.
// Those are usually bugs.
let staleAssignments = new Set();
function reportStaleAssignment(writeExpr, key) {
if (staleAssignments.has(key)) {
return;
}
staleAssignments.add(key);
context.report({
node: writeExpr,
message:
`Assignments to the '${key}' variable from inside React Hook ` +
`${context.getSource(reactiveHook)} will be lost after each ` +
`render. To preserve the value over time, store it in a useRef ` +
`Hook and keep the mutable value in the '.current' property. ` +
`Otherwise, you can move this variable directly inside ` +
`${context.getSource(reactiveHook)}.`,
});
}

// Remember which deps are optional and report bad usage first.
const optionalDependencies = new Set();
dependencies.forEach(({isStatic, references}, key) => {
if (isStatic) {
optionalDependencies.add(key);
}
references.forEach(reference => {
if (reference.writeExpr) {
reportStaleAssignment(reference.writeExpr, key);
}
});
});
if (staleAssignments.size > 0) {
// The intent isn't clear so we'll wait until you fix those first.
return;
}

let {
suggestedDependencies,
unnecessaryDependencies,
Expand Down

0 comments on commit b1cccd1

Please sign in to comment.