Warn about setState directly in dep-less useEffect (#15184)

facebook · Mar 22, 2019 · b1cccd1 · b1cccd1
1 parent 78f2775
commit b1cccd1
Show file tree

Hide file tree

Showing 2 changed files with 200 additions and 37 deletions.
diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
@@ -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: [
     {
@@ -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() {

diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
@@ -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') {
@@ -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,