[ESLint] Warn against assignments from inside Hooks (#14916)

gaearon · web-flow · commit a77bbf1a1c27 · 2019-02-21T19:23:00.000Z
* [ESLint] Warn against assignments from inside Hooks

* Include variable name

* Add a test for the legit case
diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
@@ -433,6 +433,19 @@ const tests = {
         });
       `,
     },
+    {
+      // This is not ideal but warning would likely create
+      // too many false positives. We do, however, prevent
+      // direct assignments.
+      code: `
+        function MyComponent(props) {
+          let obj = {};
+          useEffect(() => {
+            obj.foo = true;
+          }, [obj]);
+        }
+      `,
+    },
   ],
   invalid: [
     {
@@ -891,7 +904,6 @@ const tests = {
       ],
     },
     {
-      // only: true,
       code: `
         function MyComponent(props) {
           useEffect(() => {
@@ -1608,6 +1620,128 @@ const tests = {
           'Either include them or remove the dependency array.',
       ],
     },
+    {
+      code: `
+        function MyComponent(props) {
+          let value;
+          let value2;
+          let value3;
+          let asyncValue;
+          useEffect(() => {
+            value = 42;
+            value2 = 100;
+            value = 43;
+            console.log(value2);
+            console.log(value3);
+            setTimeout(() => {
+              asyncValue = 100;
+            });
+          }, []);
+        }
+      `,
+      // This is a separate warning unrelated to others.
+      // We could've made a separate rule for it but it's rare enough to name it.
+      // No autofix suggestion because the intent isn't clear.
+      output: `
+        function MyComponent(props) {
+          let value;
+          let value2;
+          let value3;
+          let asyncValue;
+          useEffect(() => {
+            value = 42;
+            value2 = 100;
+            value = 43;
+            console.log(value2);
+            console.log(value3);
+            setTimeout(() => {
+              asyncValue = 100;
+            });
+          }, []);
+        }
+      `,
+      errors: [
+        // value
+        `Assignments to the 'value' variable from inside a React useEffect Hook ` +
+          `will not persist between re-renders. ` +
+          `If it's only needed by this Hook, move the variable inside it. ` +
+          `Alternatively, declare a ref with the useRef Hook, ` +
+          `and keep the mutable value in its 'current' property.`,
+        // value2
+        `Assignments to the 'value2' variable from inside a React useEffect Hook ` +
+          `will not persist between re-renders. ` +
+          `If it's only needed by this Hook, move the variable inside it. ` +
+          `Alternatively, declare a ref with the useRef Hook, ` +
+          `and keep the mutable value in its 'current' property.`,
+        // asyncValue
+        `Assignments to the 'asyncValue' variable from inside a React useEffect Hook ` +
+          `will not persist between re-renders. ` +
+          `If it's only needed by this Hook, move the variable inside it. ` +
+          `Alternatively, declare a ref with the useRef Hook, ` +
+          `and keep the mutable value in its 'current' property.`,
+      ],
+    },
+    {
+      code: `
+        function MyComponent(props) {
+          let value;
+          let value2;
+          let value3;
+          let asyncValue;
+          useEffect(() => {
+            value = 42;
+            value2 = 100;
+            value = 43;
+            console.log(value2);
+            console.log(value3);
+            setTimeout(() => {
+              asyncValue = 100;
+            });
+          }, [value, value2, value3]);
+        }
+      `,
+      // This is a separate warning unrelated to others.
+      // We could've made a separate rule for it but it's rare enough to name it.
+      // No autofix suggestion because the intent isn't clear.
+      output: `
+        function MyComponent(props) {
+          let value;
+          let value2;
+          let value3;
+          let asyncValue;
+          useEffect(() => {
+            value = 42;
+            value2 = 100;
+            value = 43;
+            console.log(value2);
+            console.log(value3);
+            setTimeout(() => {
+              asyncValue = 100;
+            });
+          }, [value, value2, value3]);
+        }
+      `,
+      errors: [
+        // value
+        `Assignments to the 'value' variable from inside a React useEffect Hook ` +
+          `will not persist between re-renders. ` +
+          `If it's only needed by this Hook, move the variable inside it. ` +
+          `Alternatively, declare a ref with the useRef Hook, ` +
+          `and keep the mutable value in its 'current' property.`,
+        // value2
+        `Assignments to the 'value2' variable from inside a React useEffect Hook ` +
+          `will not persist between re-renders. ` +
+          `If it's only needed by this Hook, move the variable inside it. ` +
+          `Alternatively, declare a ref with the useRef Hook, ` +
+          `and keep the mutable value in its 'current' property.`,
+        // asyncValue
+        `Assignments to the 'asyncValue' variable from inside a React useEffect Hook ` +
+          `will not persist between re-renders. ` +
+          `If it's only needed by this Hook, move the variable inside it. ` +
+          `Alternatively, declare a ref with the useRef Hook, ` +
+          `and keep the mutable value in its 'current' property.`,
+      ],
+    },
   ],
 };
 
diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
@@ -186,7 +186,10 @@ export default {
           // again in our dependencies array. Remember whether it's static.
           if (!dependencies.has(dependency)) {
             const isStatic = isDefinitelyStaticDependency(reference);
-            dependencies.set(dependency, isStatic);
+            dependencies.set(dependency, {
+              isStatic,
+              reference,
+            });
           }
         }
         for (const childScope of currentScope.childScopes) {
@@ -258,6 +261,7 @@ export default {
       let unnecessaryDependencies = new Set();
       let missingDependencies = new Set();
       let actualDependencies = Array.from(dependencies.keys());
+      let foundStaleAssignments = false;
 
       function satisfies(actualDep, dep) {
         return actualDep === dep || actualDep.startsWith(dep + '.');
@@ -280,7 +284,22 @@ export default {
       });
 
       // Then fill in the missing ones.
-      dependencies.forEach((isStatic, key) => {
+      dependencies.forEach(({isStatic, reference}, key) => {
+        if (reference.writeExpr) {
+          foundStaleAssignments = true;
+          context.report({
+            node: reference.writeExpr,
+            message:
+              `Assignments to the '${key}' variable from inside a React ${context.getSource(
+                reactiveHook,
+              )} Hook ` +
+              `will not persist between re-renders. ` +
+              `If it's only needed by this Hook, move the variable inside it. ` +
+              `Alternatively, declare a ref with the useRef Hook, ` +
+              `and keep the mutable value in its 'current' property.`,
+          });
+        }
+
         if (
           !suggestedDependencies.some(suggestedDep =>
             satisfies(key, suggestedDep),
@@ -319,6 +338,11 @@ export default {
         return;
       }
 
+      if (foundStaleAssignments) {
+        // The intent isn't clear so we'll wait until you fix those first.
+        return;
+      }
+
       function getWarningMessage(deps, singlePrefix, label, fixVerb) {
         if (deps.size === 0) {
           return null;
